[Feature/extensions] Add getSettings support for AD#4519
Merged
saratvemulapalli merged 14 commits intoopensearch-project:feature/extensionsfrom Sep 20, 2022
Merged
[Feature/extensions] Add getSettings support for AD#4519saratvemulapalli merged 14 commits intoopensearch-project:feature/extensionsfrom
saratvemulapalli merged 14 commits intoopensearch-project:feature/extensionsfrom
Conversation
3 tasks
Contributor
Gradle Check (Jenkins) Run Completed with:
|
c4ea80c to
86769fc
Compare
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Member
Author
|
This is ready for review. Notes:
|
6 tasks
ryanbogan
reviewed
Sep 16, 2022
server/src/test/java/org/opensearch/extensions/ExtensionResponseTests.java
Outdated
Show resolved
Hide resolved
joshpalis
reviewed
Sep 16, 2022
server/src/main/java/org/opensearch/extensions/settings/SettingsRequestHandler.java
Outdated
Show resolved
Hide resolved
Contributor
Gradle Check (Jenkins) Run Completed with:
|
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>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
Outdated
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
Outdated
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
Outdated
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java
Outdated
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/extensions/settings/RegisterSettingsRequest.java
Outdated
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
server/src/main/java/org/opensearch/extensions/settings/SettingsRequestHandler.java
Outdated
Show resolved
Hide resolved
Member
|
Did initial pass. @dbwiddis and @saratvemulapalli do you think we should commit |
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Contributor
Gradle Check (Jenkins) Run Completed with:
|
Member
Author
I put in the minimum to handle the settings for AD, but there will be more work required in that class for the more general cases. See #148. |
owaiskazi19
reviewed
Sep 19, 2022
server/src/test/java/org/opensearch/common/settings/WriteableSettingTests.java
Show resolved
Hide resolved
owaiskazi19
reviewed
Sep 19, 2022
| assertTrue(props.contains(Property.Dynamic)); | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Great to see tests for all the types
joshpalis
approved these changes
Sep 19, 2022
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Contributor
Gradle Check (Jenkins) Run Completed with:
|
saratvemulapalli
approved these changes
Sep 20, 2022
Member
saratvemulapalli
left a comment
There was a problem hiding this comment.
Thanks @dbwiddis , I did a pass most of the changes look good to me.
6 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion PR: opensearch-project/opensearch-sdk-java#147
NOTE: Merging this PR will break
mainon SDK project until the companion PR is merged. Merge the SDK PR shortly after merging this one.Description
Registers custom Settings from an extension as dynamic settings in the Settings Module.
The pattern is similar to registering REST actions, with the complication that a
Setting<T>needs to be passed over transport, and due to type erasure,<T>cannot be determined at runtime. AWriteableSettingclass determines this type by examining the default value and implements it for all types expected in the AD extension.There will be a need to handle additional types in the future, such as List, and to track parsers and validators. This is tracked in #148
Issues Resolved
Fixes #79
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.