Update Rest status for DecommissioningFailedException by imRishN · Pull Request #5283 · opensearch-project/OpenSearch

imRishN · 2022-11-16T15:32:07Z

Signed-off-by: Rishab Nahata rnnahata@amazon.com

Description

This PR aims to update the RestStatus for DecommissioningFailedException to BadRequest

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

github-actions · 2022-11-16T16:02:18Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ✅
URL: https://build.ci.opensearch.org/job/gradle-check/6965/
CommitID: 9617ccd

github-actions · 2022-11-17T06:52:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/6990/
CommitID: adb4e55

codecov-commenter · 2022-11-17T06:55:00Z

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.11%. Comparing base (0210b76) to head (6fc8773).
⚠️ Report is 4412 commits behind head on main.

Files with missing lines	Patch %	Lines
...r/decommission/DecommissioningFailedException.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5283      +/-   ##
============================================
+ Coverage     70.91%   71.11%   +0.20%     
- Complexity    58075    58203     +128     
============================================
  Files          4711     4711              
  Lines        277534   277535       +1     
  Branches      40169    40169              
============================================
+ Hits         196802   197374     +572     
+ Misses        64582    64046     -536     
+ Partials      16150    16115      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dblock · 2022-11-17T17:47:40Z

Did we ship the decommission feature (is this an API change)? Otherwise we can put a skip-changelog on it.

dblock

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.

anshu1106 · 2022-11-18T05:55:01Z

+
+    @Override
+    public RestStatus status() {
+        return RestStatus.BAD_REQUEST;


Here https://github.com/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?

Hmm right. Will fix this in #5093 PR and would rather throw NodeClosedException

Lets break down to use specific exceptions and use 400/403 as needed in those relevant cases

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 <rnnahata@amazon.com>

imRishN · 2022-11-18T10:03:18Z

Did we ship the decommission feature (is this an API change)? Otherwise we can put a skip-changelog on it.

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 <rnnahata@amazon.com>

github-actions · 2022-11-18T10:32:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7045/
CommitID: eb35a3d
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

github-actions · 2022-11-18T11:12:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedUpdateToShardLimitsAndRejections

URL: https://build.ci.opensearch.org/job/gradle-check/7046/
CommitID: 7e7aa43
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2022-11-18T11:23:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/7047/
CommitID: 10fc0c4
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

dblock · 2022-11-18T22:26:25Z

Did we ship the decommission feature (is this an API change)? Otherwise we can put a skip-changelog on it.

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?

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.

imRishN · 2022-11-19T13:38:59Z

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 experimental in rest-api-spec and the documentation issue which is still in progress would also call that out, if that's what you mean by making the feature experimental? Please correct me if I am wrong. I didn't understand why these incremental changes cannot be part of 2.5? I'll add a change log for this PR

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

github-actions · 2022-11-19T14:08:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testCoordinatingPrimaryThreadedUpdateToShardLimitsAndRejections
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/7112/
CommitID: 1ff5063
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2022-11-25T11:31:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/7336/
CommitID: f9fc976

imRishN · 2022-11-27T20:15:30Z

@dblock @Bukhtawar can we merge this?

dblock

Let's sweat the small stuff in the CHANGELOG, pls.

dblock · 2022-11-28T18:09:34Z

 - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))
 - Fixed compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
 - Reject bulk requests with invalid actions ([#5299](https://github.com/opensearch-project/OpenSearch/issues/5299))
+- Update Rest status for DecommissioningFailedException([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))


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 <rnnahata@amazon.com>

github-actions · 2022-11-30T07:11:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/7423/
CommitID: 6fc8773

dblock · 2022-11-30T15:45:32Z

Marked for backport 2.x since the feature is experimental in 2.4.

opensearch-trigger-bot · 2022-11-30T15:46:21Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5283-to-2.x.

dblock · 2022-11-30T16:00:19Z

@imRishN Needs manual backport pls :(

…ject#5283) Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

imRishN · 2022-12-01T07:13:32Z

Created backport PR

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

Update Rest status for DecommissioningFailedException

9617ccd

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

imRishN marked this pull request as ready for review November 16, 2022 17:12

imRishN requested review from a team and reta as code owners November 16, 2022 17:12

Merge remote-tracking branch 'upstream/main' into fix-response-code

adb4e55

dblock suggested changes Nov 17, 2022

View reviewed changes

anshu1106 reviewed Nov 18, 2022

View reviewed changes

Add tests for decommission response code

2592f88

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

imRishN added 2 commits November 18, 2022 15:34

Remove unnecessary test change

19e24a8

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

Fix spotless check

eb35a3d

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

imRishN added 2 commits November 18, 2022 16:15

Merge remote-tracking branch 'upstream/main' into fix-response-code

7e7aa43

Fix

10fc0c4

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

dblock approved these changes Nov 18, 2022

View reviewed changes

Add changelog

1ff5063

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

Merge branch 'main' into fix-response-code

f9fc976

dblock suggested changes Nov 28, 2022

View reviewed changes

imRishN added 2 commits November 30, 2022 12:07

Merge remote-tracking branch 'upstream/main' into fix-response-code

2026ce4

Update changelog

6fc8773

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

dblock approved these changes Nov 30, 2022

View reviewed changes

dblock merged commit c7ba253 into opensearch-project:main Nov 30, 2022

dblock added the backport 2.x Backport to 2.x branch label Nov 30, 2022

imRishN added a commit to imRishN/OpenSearch that referenced this pull request Dec 1, 2022

Update Rest status for DecommissioningFailedException (opensearch-pro…

d1854c1

…ject#5283) Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

imRishN mentioned this pull request Dec 1, 2022

[Backport 2.x] Update Rest status for DecommissioningFailedException (#5283) #5420

Merged

6 tasks

Bukhtawar pushed a commit that referenced this pull request Dec 1, 2022

Update Rest status for DecommissioningFailedException (#5283) (#5420)

fac0613

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

Conversation

imRishN commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions Bot commented Nov 16, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Nov 17, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Nov 17, 2022 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dblock commented Nov 17, 2022

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

anshu1106 Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

imRishN Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

Bukhtawar Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

imRishN Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

imRishN commented Nov 18, 2022

Uh oh!

github-actions Bot commented Nov 18, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Nov 18, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Nov 18, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imRishN commented Nov 19, 2022

Uh oh!

github-actions Bot commented Nov 19, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Nov 25, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

imRishN commented Nov 27, 2022

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

dblock Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

imRishN Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 30, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented Nov 30, 2022

Uh oh!

opensearch-trigger-bot Bot commented Nov 30, 2022

Uh oh!

dblock commented Nov 30, 2022

Uh oh!

imRishN commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

imRishN commented Nov 16, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited by codecov Bot

Loading

dblock commented Nov 18, 2022 •

edited

Loading