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.

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

Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class #4792

Conversation

xuezhou25
Copy link
Contributor

@xuezhou25 xuezhou25 commented Oct 14, 2022

Description

Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class so that the helpful error messages can be reported.

Issues Resolved

resolve #4745

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.

…penSearchException in AbstractScopedSettings class

Signed-off-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou <[email protected]>
@xuezhou25 xuezhou25 requested review from a team and reta as code owners October 14, 2022 11:08
Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@opensearch-project opensearch-project deleted a comment from github-actions bot Oct 14, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-project opensearch-project deleted a comment from github-actions bot Oct 14, 2022
Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

*
* @opensearch.internal
*/
public class InvalidArgumentException extends OpenSearchException {
Copy link
Member

Choose a reason for hiding this comment

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

Can the existing org.opensearch.common.settings.SettingsException class be used here and avoid creating a new one?

It is important that this case returns a BAD_REQUEST error code, so the following would need to be added to SettingsException to make that happen:

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

That might be okay because it looks like the existing usages of SettingsException would probably be better as a BAD_REQUEST, but I'm not 100% sure about that.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

CHANGELOG.md Outdated
@@ -85,6 +85,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Backport Apache Lucene version change for 2.4.0 ([#4677](https://github.com/opensearch-project/OpenSearch/pull/4677))
- Refactor Base Action class javadocs to OpenSearch.API ([#4732](https://github.com/opensearch-project/OpenSearch/pull/4732))
- Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459))
- Mapping IllegalArgumentException to InvalidArgumentException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
Copy link
Member

Choose a reason for hiding this comment

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

Describe what has changed for the caller instead?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@tlfeng
Copy link
Collaborator

tlfeng commented Jan 11, 2023

In build 9282:
The failed test is introduced in commit da94dbb on 12/22/2022

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.extensions.ExtensionsManagerTests.testHandleAddSettingsUpdateConsumerRequest" -Dtests.seed=EC17246CC904E1E0 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ar-BH -Dtests.timezone=America/Inuvik -Druntime.java=19

org.opensearch.extensions.ExtensionsManagerTests > testHandleAddSettingsUpdateConsumerRequest FAILED
    SettingsException[Setting is not registered for key [falseSetting]]
        at __randomizedtesting.SeedInfo.seed([EC17246CC904E1E0:4A8CBBFBDAADD80B]:0)
        at app//org.opensearch.common.settings.AbstractScopedSettings.addSettingsUpdateConsumer(AbstractScopedSettings.java:249)
        at app//org.opensearch.common.settings.AbstractScopedSettings.addSettingsUpdateConsumer(AbstractScopedSettings.java:476)
        at app//org.opensearch.extensions.AddSettingsUpdateConsumerRequestHandler.handleAddSettingsUpdateConsumerRequest(AddSettingsUpdateConsumerRequestHandler.java:73)
        at app//org.opensearch.extensions.ExtensionsManagerTests.testHandleAddSettingsUpdateConsumerRequest(ExtensionsManagerTests.java:738)

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: null ❌
  • URL: null
  • CommitID: 07f1f72
    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?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@tlfeng
Copy link
Collaborator

tlfeng commented Jan 12, 2023

In build 9355, the same failure as above #4792 (comment)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal
Copy link
Member

@xuezhou25 Can you please rebase your branch with the latest commits?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Xue Zhou <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Gradle Check (Jenkins) Run Completed with:

@xuezhou25 xuezhou25 merged commit ee8105b into opensearch-project:main Feb 6, 2023
@xuezhou25 xuezhou25 deleted the map_IllegalArgumentException_to_OpenSearchException branch February 6, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the AbstractScopedSettings class to throw OpenSearchExceptions instead of IllegalArgumentException
7 participants