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

[Feature/extensions] Add getSettings support for AD #4519

Merged

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Sep 15, 2022

Companion PR: opensearch-project/opensearch-sdk-java#147

NOTE: Merging this PR will break main on 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. A WriteableSetting class 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

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

@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:

@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:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dbwiddis
Copy link
Member Author

This is ready for review. Notes:

CC: @saratvemulapalli @owaiskazi19

@github-actions
Copy link
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
Copy link
Member

Did initial pass. @dbwiddis and @saratvemulapalli do you think we should commit WriteableSetting to main?

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dbwiddis
Copy link
Member Author

Did initial pass. @dbwiddis and @saratvemulapalli do you think we should commit WriteableSetting to main?

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.

assertTrue(props.contains(Property.Dynamic));
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Great to see tests for all the types

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Small Nit. Rest LGTM!

@joshpalis joshpalis self-requested a review September 19, 2022 20:43
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

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 , I did a pass most of the changes look good to me.

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.

6 participants