Update Rest status for DecommissioningFailedException #5283
Conversation
Signed-off-by: Rishab Nahata <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5283 +/- ##
=========================================
Coverage 70.95% 70.95%
- Complexity 58156 58172 +16
=========================================
Files 4711 4711
Lines 277533 277534 +1
Branches 40169 40169
=========================================
+ Hits 196924 196926 +2
+ Misses 64510 64497 -13
- Partials 16099 16111 +12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Did we ship the decommission feature (is this an API change)? Otherwise we can put a skip-changelog on it. |
There was a problem hiding this comment.
Checking for .status(), especially by casting the exception, doesn't actually check that the end user is getting a 400. It's pretty obvious that DecommissioningFailedException will always have a status of BAD_REQUEST. I think you should write an actual REST test that makes an API call and gets a 400.
|
|
||
| @Override | ||
| public RestStatus status() { | ||
| return RestStatus.BAD_REQUEST; |
There was a problem hiding this comment.
Here https:/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java#L271 it seems its not a bad request due to which the exception is thrown. Should we use a different exception over here?
There was a problem hiding this comment.
Hmm right. Will fix this in #5093 PR and would rather throw NodeClosedException
There was a problem hiding this comment.
Lets break down to use specific exceptions and use 400/403 as needed in those relevant cases
There was a problem hiding this comment.
This PR just attempts to change the status code for DecommissioningFailedException and here we are making it 400. Once case which Anshu mentioned where we could rather throw NodeClosedException will be fixed in a subsequent PR.
Signed-off-by: Rishab Nahata <[email protected]>
Yes, decommission has 3 new APIs and is part of 2.4. We are still incrementally enhancing it. In this, we need to add a change log to this PR? |
Signed-off-by: Rishab Nahata <[email protected]>
Signed-off-by: Rishab Nahata <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishab Nahata <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Well, 2.4 has shipped, and that feature is not experimental, or is it? Now you're changing the API in a backwards incompatible way, so I think this cannot go into 2.5 but I am not sure since before it would fail with a 500 which is generic. Either way it does need a CHANGELOG entry in the right version as part of this PR. |
We have labelled it |
Signed-off-by: Rishab Nahata <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
@dblock @Bukhtawar can we merge this? |
CHANGELOG.md
Outdated
| @@ -77,6 +77,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
| - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https:/opensearch-project/OpenSearch/pull/4827)) | |||
| - Fixed compression support for h2c protocol ([#4944](https:/opensearch-project/OpenSearch/pull/4944)) | |||
| - Reject bulk requests with invalid actions ([#5299](https:/opensearch-project/OpenSearch/issues/5299)) | |||
| - Update Rest status for DecommissioningFailedException([#5283](https:/opensearch-project/OpenSearch/pull/5283)) | |||
There was a problem hiding this comment.
So this is not a fix, but a change. Move above and say "Changed htto code for ... from 500 to 400" similar to what we did for NotXContentException
Also add a space before ( like for all other lines, please.
Signed-off-by: Rishab Nahata <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
|
Marked for backport 2.x since the feature is experimental in 2.4. |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5283-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c7ba2538e837507d0decc77aaba2fa21745fdac1
# Push it to GitHub
git push --set-upstream origin backport/backport-5283-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.xThen, create a pull request where the |
|
@imRishN Needs manual backport pls :( |
…ject#5283) Signed-off-by: Rishab Nahata <[email protected]>
|
Created backport PR |
Signed-off-by: Rishab Nahata <[email protected]>
Signed-off-by: Rishab Nahata [email protected]
Description
This PR aims to update the RestStatus for
DecommissioningFailedExceptiontoBadRequestIssues Resolved
#5075
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.