[Feature/extensions] Modified Environment settings support by joshpalis · Pull Request #4731 · opensearch-project/OpenSearch

joshpalis · 2022-10-10T22:04:35Z

Description

Environment Settings support for extensibility will now pass the entire Settings object from OpenSearch node environment to the SDK.

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

Issues Resolved

opensearch-project/opensearch-sdk-java#78

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.

… Refactored response to include the entire environment settings object from node.java Signed-off-by: Joshua Palis <jpalis@amazon.com>

Signed-off-by: Joshua Palis <jpalis@amazon.com>

github-actions · 2022-10-10T22:27:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4335/
CommitID: 82bc945

github-actions · 2022-10-10T22:30:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4333/
CommitID: 028a2ad

codecov-commenter · 2022-10-10T22:32:37Z

Codecov Report

Merging #4731 (60b8ff3) into feature/extensions (eee59c5) will increase coverage by 0.21%.
The diff coverage is 77.02%.

❗ Current head 60b8ff3 differs from pull request most recent head b8fa39b. Consider uploading reports for the commit b8fa39b to get more accurate results

@@                   Coverage Diff                    @@
##             feature/extensions    #4731      +/-   ##
========================================================
+ Coverage                 70.52%   70.73%   +0.21%     
+ Complexity                57898    57846      -52     
========================================================
  Files                      4708     4706       -2     
  Lines                    278528   277831     -697     
  Branches                  40661    40389     -272     
========================================================
+ Hits                     196420   196515      +95     
+ Misses                    65740    65093     -647     
+ Partials                  16368    16223     -145

Impacted Files	Coverage Δ
...main/java/org/opensearch/gradle/PublishPlugin.java	`43.43% <0.00%> (-0.45%)`	⬇️
.../java/org/opensearch/gradle/pluginzip/Publish.java	`11.62% <0.00%> (-0.57%)`	⬇️
...earch/analysis/common/NGramTokenFilterFactory.java	`75.00% <0.00%> (+10.71%)`	⬆️
...gregations/bucket/geogrid/BucketPriorityQueue.java	`75.00% <ø> (ø)`
...arch/aggregations/bucket/geogrid/CellIdSource.java	`69.23% <ø> (ø)`
...ions/bucket/geogrid/GeoGridAggregationBuilder.java	`75.67% <ø> (ø)`
.../bucket/geogrid/GeoHashGridAggregationBuilder.java	`100.00% <ø> (ø)`
.../bucket/geogrid/GeoTileGridAggregationBuilder.java	`100.00% <ø> (ø)`
...ions/bucket/geogrid/InternalGeoHashGridBucket.java	`100.00% <ø> (ø)`
...ions/bucket/geogrid/InternalGeoTileGridBucket.java	`100.00% <ø> (ø)`
... and 663 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-10-10T22:33:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4334/
CommitID: b761f3b

github-actions · 2022-10-10T23:05:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4338/
CommitID: 82bc945

dbwiddis · 2022-10-11T00:27:06Z

server/src/main/java/org/opensearch/env/EnvironmentSettingsResponse.java

    }

    public EnvironmentSettingsResponse(StreamInput in) throws IOException {
        super(in);


Nit: I don't like seeing super(in) (which in the Response case is a no-op) without a corresponding out.writeTo(). Makes me think too much. :)

owaiskazi19 · 2022-10-11T19:16:21Z

server/src/test/java/org/opensearch/extensions/ExtensionsOrchestratorTests.java

-        assertTrue(responseSettings.containsAll(componentSettings));
-        assertTrue(componentSettings.containsAll(responseSettings));
+        EnvironmentSettingsResponse environmentSettingsResponse = new EnvironmentSettingsResponse(settings);
+        assertEquals(settings, environmentSettingsResponse.getEnvironmentSettings());


We should add a test to compare if the setting present in the list matches the one we have added to the list

With this modification to Environment settings support, we are no longer transporting a list of Setting<T> objects but rather an entire Settings object, so we are not adding anything to any list. As for the comparison test for this transported Settings object, this is handled on line 501.

…t Setting<T> objects will return either the registered setting value from the Settings object, or the default value of the Setting<T> object Signed-off-by: Joshua Palis <jpalis@amazon.com>

Signed-off-by: Joshua Palis <jpalis@amazon.com>

…st for better clarity Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis · 2022-10-12T19:19:15Z

Summary of EnvironmentSettings workflow modification for future reference :

Previous Design :

Extensions will send OpenSearch a list of Setting<T> objects that it would want to retrieve values from. The following workflow will achieve this support :

Pass in environment settings from Node.java to ExtensionsOrchestrator during node initialization
During createComponents, for each component, the Extension will send a request to OpenSearch containing a List of Setting objects
OpenSearch will receive this list and invoke Setting.get(environmentSettings) to retrieve the corresponding values (or default value if the setting object is not yet registered) and map them to the setting
OpenSearch will respond with the Map<Setting setting, Object value>
The extension will then invoke Map.get(Setting) to retrieve the value of the setting from the OpenSearch environment and then initialize the corresponding component field with this value

New Design :

While performing create-component integration for Anomaly Detection, it was determined that the entire environment Settings is necessary to transport to the extension in order to initialize the JvmService object here which is then further used to initialize the ADCircuitBreakerService, MemoryTracker, ModelManager , and HashRing objects. These objects are then used to create the ADTaskManager component. Since we must transport this Settings object in its entirety, this makes sending List<Setting<T>> objects to OpenSearch for environment settings values obsolete. Thus the following workflow has been modified as such :

Pass in environment settings from Node.java to ExtensionsOrchestrator during node initialization
During createComponents, a single sendEnvironmentSettingsRequest will be sent to OpenSearch to retrieve the Settings object from ExtensionsOrchestrator.
Settings object will then be passed to the create-components method of Extension.java, which it will be further passed to any of the objects / components that need it.
Components will then use the Settings object to determine the registered or default value of the Setting<T> object associated with the component field. For example : https://github.com/joshpalis/anomaly-detection/blob/572fd8d4f08691326d92535f88f7d5f06ea4ea63/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java#L155

github-actions · 2022-10-12T19:28:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4511/
CommitID: 7efadd6

github-actions · 2022-10-12T19:32:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4513/
CommitID: 60b8ff3

github-actions · 2022-10-12T19:34:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4514/
CommitID: b8fa39b

owaiskazi19

Thanks for addressing the comments!

github-actions · 2022-10-12T19:42:38Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-10-12T20:15:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4520/
CommitID: b8fa39b

joshpalis added 2 commits October 10, 2022 20:46

Removed environment settings request, replaced with extensionRequest.…

0923fbc

… Refactored response to include the entire environment settings object from node.java Signed-off-by: Joshua Palis <jpalis@amazon.com>

changelog placeholder

028a2ad

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis requested review from a team and reta as code owners October 10, 2022 22:04

Updating changelog

b761f3b

Signed-off-by: Joshua Palis <jpalis@amazon.com>

joshpalis mentioned this pull request Oct 10, 2022

[Feature/extensions] Modified Environment settings support opensearch-project/opensearch-sdk-java#179

Merged

rebasing with feature/extensions

82bc945

Signed-off-by: Joshua Palis <jpalis@amazon.com>

dbwiddis approved these changes Oct 11, 2022

View reviewed changes

owaiskazi19 reviewed Oct 11, 2022

View reviewed changes

joshpalis added 3 commits October 12, 2022 18:58

Updated EnvironmentSettings unit tests, added two tests to ensure tha…

7efadd6

…t Setting<T> objects will return either the registered setting value from the Settings object, or the default value of the Setting<T> object Signed-off-by: Joshua Palis <jpalis@amazon.com>

re-triggering to re-run precommit

60b8ff3

Signed-off-by: Joshua Palis <jpalis@amazon.com>

Updating registered setting name iEnvironmentSettingsDefault value te…

b8fa39b

…st for better clarity Signed-off-by: Joshua Palis <jpalis@amazon.com>

owaiskazi19 approved these changes Oct 12, 2022

View reviewed changes

owaiskazi19 merged commit dfeefa6 into opensearch-project:feature/extensions Oct 12, 2022

ryanbogan mentioned this pull request Nov 17, 2022

[Feature/extensions] Merge to main tracking - REST, TranportActions, and EnvironmentSettings updates (up to present) #5273

Closed

Conversation

joshpalis commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dbwiddis Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

joshpalis Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshpalis commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

owaiskazi19 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshpalis commented Oct 10, 2022 •

edited

Loading

codecov-commenter commented Oct 10, 2022 •

edited

Loading

joshpalis Oct 11, 2022 •

edited

Loading

joshpalis commented Oct 12, 2022 •

edited

Loading