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

Add getSettings support for AD #147

Merged
merged 6 commits into from
Sep 20, 2022

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Sep 19, 2022

Companion PR: opensearch-project/OpenSearch#4519

NOTE: Gradle Check is failing due to new classes on the Companion PR. Check completes locally when the other PR branch is published to Maven Local. Re-run check after merging the companion PR.

NOTE: Merging the compainion PR will break main on SDK project until this PR (or at least the first commit from it) is merged. Merge this PR shortly after merging the companion PR.

Description

Registers custom Settings from an extension as dynamic settings in the Settings Module.

The pattern is similar to registering REST actions.

Recommend reviewers evaluate the commits separately to simplify review. Explanation follows:

  • The companion PR combined RegisterRestActionsResponse and a response for the settings to ExtensionStringResponse. Merging the companion PR will break main; this first commit fixes the rename.
  • Since we have a ExtensionStringResponseHandler it made sense to rename the handler for the boolean response to align.
  • Adding the static modifier in Add ActionListener onFailure to ExtensionsRunner #87 created compiler warnings for accessing static methods non-statically. I undid that change.
  • The fourth commit actually sends the settings to OpenSearch and is the main point of this PR!
  • I fixed up some javadocs.

Issues Resolved

Fixes #79

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: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis requested a review from a team September 19, 2022 01:05
@dbwiddis dbwiddis changed the title Get settings Add getSettings support for AD Sep 19, 2022
owaiskazi19
owaiskazi19 previously approved these changes Sep 19, 2022
joshpalis
joshpalis previously approved these changes Sep 19, 2022
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Thanks @dbwiddis !!

@joshpalis joshpalis merged commit 9e977c1 into opensearch-project:main Sep 20, 2022
@dbwiddis dbwiddis deleted the get-settings branch September 29, 2022 06:18
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
* Change RegisterRestActionsResponse to ExtensionStringResponse

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Rename ExtensionResponseHandler to ExtensionBooleanResponseHandler

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix compiler warning on incorrect static modifier

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Send RegisterSettingsRequest to OpenSearch

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix javadoc warnings

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* s/Setting/CustomSetting/g

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Signed-off-by: Daniel Widdis <widdis@gmail.com>
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.

[Meta] [FEATURE] Add getSettings support for AD
4 participants