[Feature/extensions] Adding support to register settings dynamically #3753

saratvemulapalli · 2022-07-01T00:05:58Z

Description

Adding support to register dynamic settings.
For plugins/extensions, they can define their own settings which are propagated across the cluster.
This PR adds support to register Node Scope or Index Scope settings at anytime.
This feature support unlocks a part of extensions hot swap.

This is a recreate of PR: #3529

Issues Resolved

#3434

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

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: Sarat Vemulapalli <[email protected]>

saratvemulapalli · 2022-07-01T00:07:01Z

Tagging: @kartg (follow up of our conversation)

github-actions · 2022-07-01T00:30:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/113/
CommitID: a11bedc

dblock · 2022-07-05T16:31:05Z

Any downsides for this implementation at runtime? I hope we can get this onto main.

saratvemulapalli · 2022-07-05T19:00:59Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌

URL: https://build.ci.opensearch.org/job/gradle-check/113/

CommitID: a11bedc

This is a bug #2772 on feature/extensions branch.

Tests with failures:
 - org.opensearch.cluster.remote.test.RemoteClustersIT.testSniffModeConnectionFails

saratvemulapalli · 2022-07-05T19:02:37Z

Any downsides for this implementation at runtime? I hope we can get this onto main.

I am looking for feedback as well. As far as I understand it feels safe enough.
I would love to get this to main.

server/src/main/java/org/opensearch/common/settings/SettingsModule.java

server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java

server/src/main/java/org/opensearch/common/settings/SettingsModule.java

Signed-off-by: Sarat Vemulapalli <[email protected]>

github-actions · 2022-07-13T01:04:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/493/
CommitID: d226611

dbwiddis

A few more comments inline. It looks like this is still a work in progress so holding off on clicking "approve".

server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java

server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java

Signed-off-by: Sarat Vemulapalli <[email protected]>

github-actions · 2022-07-14T18:18:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/624/
CommitID: 7c4dfd1

Signed-off-by: Sarat Vemulapalli <[email protected]>

github-actions · 2022-07-15T19:52:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/700/
CommitID: 4e2bc8b

dbwiddis

LGTM!

dbwiddis · 2022-07-16T00:48:45Z

Gradle check failing on Jenkins. I think I read somewhere that Jenkins configuration has changed, so that's unrelated to this PR ~~but probably should hold off merging until we get that updated.~~

kartg · 2022-07-20T22:53:03Z

server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java

@@ -144,6 +144,15 @@ protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings,
        settingUpdaters.addAll(other.settingUpdaters);
    }

+    protected boolean registerSetting(Setting<?> setting) {


Given that ClusterSettings and IndexScopedSettings are the only two subclasses of this class, why not simply make this method public and avoid the super calls in two places?

kartg · 2022-07-20T22:57:54Z

server/src/main/java/org/opensearch/common/settings/SettingsModule.java

+            if (setting.hasNodeScope()) {
+                return clusterSettings.registerSetting(setting);
+            }
+            if (setting.hasIndexScope()) {


Nitpick - else if here, unless it's possible for a setting to be both NodeScope and IndexScope

kartg · 2022-07-20T22:59:34Z

server/src/main/java/org/opensearch/common/settings/SettingsModule.java

+            if (setting.hasIndexScope()) {
+                return indexScopedSettings.registerSetting(setting);
+            }
+            logger.info("Registered new Setting: " + setting.getKey() + " successfully ");


Nitpick - This log line will be printed inconsistently since there are return statements above. Consider moving this to the caller, or before the return statements

Thanks for this. The return came later after the feedback.
Sure will send out a follow up.

saratvemulapalli added 3 commits June 30, 2022 16:41

Adding support for dynamically registering settings

26ac056

Signed-off-by: Sarat Vemulapalli <[email protected]>

Adding spotless changes

d42f405

Signed-off-by: Sarat Vemulapalli <[email protected]>

Converting complexMatchers to modifiable Map

a11bedc

Signed-off-by: Sarat Vemulapalli <[email protected]>

saratvemulapalli requested review from a team and reta as code owners July 1, 2022 00:05

saratvemulapalli requested review from dblock, dbwiddis, Bukhtawar, owaiskazi19, ryanbogan and joshpalis July 1, 2022 00:07

dblock reviewed Jul 7, 2022

View reviewed changes

server/src/main/java/org/opensearch/common/settings/SettingsModule.java Show resolved Hide resolved

server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java Outdated Show resolved Hide resolved

dbwiddis reviewed Jul 8, 2022

View reviewed changes

saratvemulapalli added 2 commits July 11, 2022 16:21

Merge branch 'feature/extensions' into dynamic-settings

66697c2

Addressing comments

d226611

Signed-off-by: Sarat Vemulapalli <[email protected]>

saratvemulapalli requested a review from dbwiddis July 13, 2022 17:14

dbwiddis reviewed Jul 14, 2022

View reviewed changes

server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java Show resolved Hide resolved

server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java Show resolved Hide resolved

Addressing comments

7c4dfd1

Signed-off-by: Sarat Vemulapalli <[email protected]>

Adding feedback loop

4e2bc8b

Signed-off-by: Sarat Vemulapalli <[email protected]>

saratvemulapalli requested a review from dbwiddis July 15, 2022 19:34

dbwiddis approved these changes Jul 16, 2022

View reviewed changes

saratvemulapalli merged commit 9b29039 into opensearch-project:feature/extensions Jul 16, 2022

kartg reviewed Jul 20, 2022

View reviewed changes

owaiskazi19 mentioned this pull request Sep 14, 2022

[Meta] [FEATURE] Add getSettings support for AD opensearch-project/opensearch-sdk-java#79

Closed

3 tasks

ryanbogan mentioned this pull request Nov 17, 2022

[Feature/extensions] Merge to main tracking - Initial extensions framework #5264

Closed

saratvemulapalli mentioned this pull request Dec 6, 2022

Merge first batch of feature/extensions into main #5347

Merged

6 tasks

ryanbogan mentioned this pull request Dec 8, 2022

Adding support to register settings dynamically #5495

Merged

6 tasks

saratvemulapalli mentioned this pull request May 9, 2023

[Extensions] Create extension point to be able to register security settings into extensions.yml opensearch-project/security#2746

Closed

owaiskazi19 mentioned this pull request May 19, 2023

[PROPOSAL] Language Analyzer plugin as an extension opensearch-project/opensearch-sdk-java#766

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/extensions] Adding support to register settings dynamically #3753

[Feature/extensions] Adding support to register settings dynamically #3753

saratvemulapalli commented Jul 1, 2022

saratvemulapalli commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

dblock commented Jul 5, 2022

saratvemulapalli commented Jul 5, 2022

Gradle Check (Jenkins) Run Completed with:

saratvemulapalli commented Jul 5, 2022

github-actions bot commented Jul 13, 2022

dbwiddis left a comment

github-actions bot commented Jul 14, 2022

github-actions bot commented Jul 15, 2022

dbwiddis left a comment

dbwiddis commented Jul 16, 2022 •

edited

Loading

kartg Jul 20, 2022

kartg Jul 20, 2022

saratvemulapalli Jul 20, 2022

kartg Jul 20, 2022

saratvemulapalli Jul 20, 2022

[Feature/extensions] Adding support to register settings dynamically #3753

[Feature/extensions] Adding support to register settings dynamically #3753

Conversation

saratvemulapalli commented Jul 1, 2022

Description

Issues Resolved

Check List

saratvemulapalli commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

dblock commented Jul 5, 2022

saratvemulapalli commented Jul 5, 2022

Gradle Check (Jenkins) Run Completed with:

saratvemulapalli commented Jul 5, 2022

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 14, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis commented Jul 16, 2022 • edited Loading

kartg Jul 20, 2022

Choose a reason for hiding this comment

kartg Jul 20, 2022

Choose a reason for hiding this comment

saratvemulapalli Jul 20, 2022

Choose a reason for hiding this comment

kartg Jul 20, 2022

Choose a reason for hiding this comment

saratvemulapalli Jul 20, 2022

Choose a reason for hiding this comment

dbwiddis commented Jul 16, 2022 •

edited

Loading