Use status code 500 for errors if no shard failed #88551
Conversation
If there is no shard failure we would like to use a generic Internal Server Error other than a Service Unavailable. Moreover, if the cause is known we use a status code that reflects the cause. Resolves elastic#20004
salvatore-campagna
added
>bug
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @salvatore-campagna, I've created a changelog YAML for you. |
salvatore-campagna
changed the title
There was a problem hiding this comment.
Generally the code looks good. I have left a minor comment.
However, I must admit I am not an expert on the related code. So @nik9000 or @not-napoleon could have a better opinion here.
| RestStatus status = shardFailures[0].status(); | ||
| if (shardFailures.length > 1) { | ||
| for (int i = 1; i < shardFailures.length; i++) { | ||
| if (shardFailures[i].status().getStatus() >= 500) { |
There was a problem hiding this comment.
I am not a big fan of the hardcoded 500 value here.
Is there any way to add a constant such as RestStatus.INTERNAL_SERVER_ERROR.getStatus()
Also, should you perhaps delegate this to the RestStatus.status() method?
Would running something like RestStatus.status(0, shardFailures.length, shardFailures) return the same result? Again, I am not an expert on this part of the code, so take this with a grain of salt.
| @@ -161,4 +161,8 @@ public String toString() { | |||
| public String getPhaseName() { | |||
| return phaseName; | |||
| } | |||
|
|
|||
| public ShardSearchFailure[] getShardFailures() { | |||
There was a problem hiding this comment.
I think this could be package scoped?
There was a problem hiding this comment.
It looks like there is already a method shardFailures() which I didn't see...so I will use the existing one and get rid of the one I added.
| } | ||
| } | ||
| return status; | ||
| return super.status(); |
|
Heya, have we thought about possible consequences in clients code around changing the status code here? I've seen this is marked bug hence it is not considered a breaking change, I assume. But was this discussed before merging? |
If there is no shard failure we would like to use a generic
Internal Server Error other than a Service Unavailable.
Moreover, if the cause is known we use a status code that
reflects the cause.
Resolves #20004