Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Rest status for DecommissioningFailedException #5283

Merged
merged 11 commits into from
Nov 30, 2022
Merged

Update Rest status for DecommissioningFailedException #5283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9617ccd
Update Rest status for DecommissioningFailedException
imRishN Nov 16, 2022
adb4e55
Merge remote-tracking branch 'upstream/main' into fix-response-code
imRishN Nov 17, 2022
2592f88
Add tests for decommission response code
imRishN Nov 18, 2022
19e24a8
Remove unnecessary test change
imRishN Nov 18, 2022
eb35a3d
Fix spotless check
imRishN Nov 18, 2022
7e7aa43
Merge remote-tracking branch 'upstream/main' into fix-response-code
imRishN Nov 18, 2022
10fc0c4
Fix
imRishN Nov 18, 2022
1ff5063
Add changelog
imRishN Nov 19, 2022
f9fc976
Merge branch 'main' into fix-response-code
imRishN Nov 25, 2022
2026ce4
Merge remote-tracking branch 'upstream/main' into fix-response-code
imRishN Nov 30, 2022
6fc8773
Update changelog
imRishN Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Update Rest status for DecommissioningFailedException 
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
  • Loading branch information
@imRishN
imRishN committed Nov 16, 2022
commit 9617ccd554693d808e73ec0c0de9f3d3aefa014d
6 changes: 6 additions & 0 deletions server/src/main/java/org/opensearch/cluster/decommission/DecommissioningFailedException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.opensearch.OpenSearchException;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.rest.RestStatus;

import java.io.IOException;

Expand Down Expand Up @@ -52,4 +53,9 @@ public void writeTo(StreamOutput out) throws IOException {
public DecommissionAttribute decommissionAttribute() {
return decommissionAttribute;
}

@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
6 changes: 6 additions & 0 deletions server/src/test/java/org/opensearch/cluster/decommission/DecommissionServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.opensearch.common.settings.ClusterSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.rest.RestStatus;
import org.opensearch.test.ClusterServiceUtils;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.transport.MockTransport;
Expand Down Expand Up @@ -140,6 +141,7 @@ public void onResponse(DecommissionResponse decommissionResponse) {
public void onFailure(Exception e) {
assertTrue(e instanceof DecommissioningFailedException);
assertThat(e.getMessage(), Matchers.endsWith("invalid awareness attribute requested for decommissioning"));
assertEquals(((DecommissioningFailedException) e).status(), RestStatus.BAD_REQUEST);
countDownLatch.countDown();
}
};
Expand Down Expand Up @@ -167,6 +169,7 @@ public void onFailure(Exception e) {
+ "Set forced awareness values before to decommission"
)
);
assertEquals(((DecommissioningFailedException) e).status(), RestStatus.BAD_REQUEST);
countDownLatch.countDown();
}
};
Expand All @@ -192,6 +195,7 @@ public void onFailure(Exception e) {
e.getMessage(),
Matchers.containsString("weight for decommissioned attribute is expected to be [0.0] but found [1.0]")
);
assertEquals(((DecommissioningFailedException) e).status(), RestStatus.BAD_REQUEST);
countDownLatch.countDown();
}
};
Expand All @@ -217,6 +221,7 @@ public void onFailure(Exception e) {
"no weights are set to the attribute. Please set appropriate weights before triggering decommission action"
)
);
assertEquals(((DecommissioningFailedException) e).status(), RestStatus.BAD_REQUEST);
countDownLatch.countDown();
}
};
Expand Down Expand Up @@ -254,6 +259,7 @@ public void onFailure(Exception e) {
} else {
assertThat(e.getMessage(), Matchers.endsWith("is in progress, cannot process this request"));
}
assertEquals(((DecommissioningFailedException) e).status(), RestStatus.BAD_REQUEST);
countDownLatch.countDown();
}
};
Expand Down