[Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… #3449

ryanbogan · 2022-05-26T17:24:19Z

…estrator

Signed-off-by: Ryan Bogan rbogan@amazon.com

Description

Adds unit tests and reads extensions from file automatically in ExtensionsOrchestrator.

Issues Resolved

3408

Check List

[ X] New functionality includes testing.
- [ X] All tests pass
[ X] 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.

…estrator Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-05-26T17:48:19Z

❌ Gradle Check failure 8dedaa9
Log 5611

Reports 5611

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

saratvemulapalli

Did a single pass.
Mostly the changes look good. Lets take care of the comments and I'll do another pass today.
Thanks @ryanbogan

saratvemulapalli · 2022-06-06T17:43:05Z

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

    TransportService transportService;
-    final DiscoveryNode extensionNode;
+    DiscoveryNode defaultExtensionNode;


Do we still need this ?
We do not need a default Extension node, we should always walk through all extensions when invoking extension points.

I just set the defaultExtensionNode to the first extension read because we need it for IndicesModuleRequest

The first extension should not be special :).
With the code we had, it was hacky to enable our manual testing.

We dont need this any more.
For every extension point we should walk through every extension.

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

dbwiddis · 2022-06-06T18:25:13Z

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

                        extensionsSet.add(
                            new DiscoveryExtension(
                                extension.getName(),
-                                "id",
+                                extension.getName(),


Are we simply using the name as the persistent ID? Or is this a placeholder?

Should it not be extension.getId()?

It's just a placeholder for now, but I'll try to switch it over since that makes more sense

dbwiddis · 2022-06-06T18:50:15Z

server/src/test/resources/config/extensions.yml

@@ -3,11 +3,11 @@ extensions:
      ephemeralId: uniqueid1


This isn't part of this PR, but isn't the ephemeralId auto-assigned, and should this be the (unique) persistent id?

Yeah I'll change it

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-13T22:09:40Z

❌ Gradle Check failure 0862a65
Log 5948

Reports 5948

owaiskazi19

LGTM! 🚢

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

…fied unit tests accordingly Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-22T22:41:21Z

❌ Gradle Check failure 31bb9d7
Log 6234

Reports 6234

saratvemulapalli

Thanks @ryanbogan for these changes.
The changes look good to me.
Some minor comments, few of them could be taken care in follow up PRs.

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

saratvemulapalli · 2022-06-22T23:47:59Z

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

+                                    new TransportAddress(TransportAddress.META_ADDRESS, Integer.parseInt(extension.getPort())),
+                                    new HashMap<String, String>(),
+                                    Version.fromString(extension.getVersion()),
+                                    pluginInfo


I honestly dont remember why we are storing plugin Info.
I think we should be able to get rid of walking through pluginDirs

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-23T00:45:37Z

❌ Gradle Check failure 8ff3f26
Log 6238

Reports 6238

dbwiddis · 2022-06-23T17:48:34Z

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

+                                new DiscoveryExtension(
+                                    extension.getName(),
+                                    extension.getUniqueId(),
+                                    // placeholder for ephemeral id, will change with POC discovery


If we will always have a unique ephemeral id, why are we using a set?

It worked better with the findPluginDirectories method that we were using to find extensions. In another PR that I will hopefully raise later today, I will be removing all of the logic related to pluginDirs so I could change it to a list if that's what you recommend

server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-23T21:42:30Z

❌ Gradle Check failure d187841
Log 6269

Reports 6269

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-24T16:15:39Z

❌ Gradle Check failure b169ec6
Log 6303

Reports 6303

owaiskazi19 · 2022-06-24T19:15:15Z

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

+            .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)
+            .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString())
+            .build();
+        ExtensionsOrchestrator orchestrator = new ExtensionsOrchestrator(settings, extensionDir);


nit: orchestrator -> extenionsOrchestrator

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-ci-bot · 2022-06-24T19:34:20Z

❌ Gradle Check failure 3bb299b
Log 6309

Reports 6309

Added unit tests and modified extension read logic for ExtensionsOrch…

8dedaa9

…estrator Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan requested review from a team and reta as code owners May 26, 2022 17:24

ryanbogan requested review from joshpalis, owaiskazi19 and saratvemulapalli May 26, 2022 17:24

joshpalis approved these changes May 26, 2022

View reviewed changes

saratvemulapalli changed the title ~~Added unit tests and modified extension read logic for ExtensionsOrch…~~ [Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… May 31, 2022

owaiskazi19 reviewed Jun 1, 2022

View reviewed changes

server/src/test/java/org/opensearch/extensions/ExtensionsOrchestratorTests.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Jun 1, 2022

View reviewed changes

server/src/test/java/org/opensearch/extensions/ExtensionsOrchestratorTests.java Show resolved Hide resolved

owaiskazi19 reviewed Jun 1, 2022

View reviewed changes

server/src/test/java/org/opensearch/extensions/ExtensionsOrchestratorTests.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Jun 1, 2022

View reviewed changes

server/src/test/java/org/opensearch/extensions/ExtensionsOrchestratorTests.java Show resolved Hide resolved

saratvemulapalli reviewed Jun 6, 2022

View reviewed changes

dbwiddis reviewed Jun 6, 2022

View reviewed changes

Fixed minor errors

0862a65

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

owaiskazi19 approved these changes Jun 13, 2022

View reviewed changes

Commit for debugging purposes

c006cc0

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

This was referenced Jun 21, 2022

Unit Tests for createComponent opensearch-project/opensearch-sdk-java#25

Closed

[Feature/extensions] Integrated CreateComponent extensionPoint #3265

Merged

ryanbogan added 2 commits June 22, 2022 21:49

Merge branch 'feature/extensions' into extensions_yml

c06e806

Changed how extensions.yml is read by ExtensionsOrchestrator and modi…

31bb9d7

…fied unit tests accordingly Signed-off-by: Ryan Bogan <rbogan@amazon.com>

saratvemulapalli reviewed Jun 23, 2022

View reviewed changes

Addressed PR Comments

8ff3f26

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

dbwiddis reviewed Jun 23, 2022

View reviewed changes

Addressed PR Comments

d187841

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

Fixed minor errors

b169ec6

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

owaiskazi19 reviewed Jun 24, 2022

View reviewed changes

Addressed PR Comments

3bb299b

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan merged commit 7625fce into opensearch-project:feature/extensions Jun 24, 2022

ryanbogan deleted the orchestrator_tests branch June 24, 2022 20:17

ryanbogan mentioned this pull request Nov 17, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… #3449

[Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… #3449

ryanbogan commented May 26, 2022

opensearch-ci-bot commented May 26, 2022

saratvemulapalli left a comment

saratvemulapalli Jun 6, 2022

ryanbogan Jun 13, 2022

saratvemulapalli Jun 14, 2022

dbwiddis Jun 6, 2022 •

edited

Loading

ryanbogan Jun 13, 2022

dbwiddis Jun 6, 2022

ryanbogan Jun 13, 2022

opensearch-ci-bot commented Jun 13, 2022

owaiskazi19 left a comment

opensearch-ci-bot commented Jun 22, 2022

saratvemulapalli left a comment

saratvemulapalli Jun 22, 2022

opensearch-ci-bot commented Jun 23, 2022

dbwiddis Jun 23, 2022

ryanbogan Jun 23, 2022

opensearch-ci-bot commented Jun 23, 2022

opensearch-ci-bot commented Jun 24, 2022

owaiskazi19 Jun 24, 2022

opensearch-ci-bot commented Jun 24, 2022

[Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… #3449

[Feature/extensions] Added unit tests and modified extension read logic for ExtensionsOrch… #3449

Conversation

ryanbogan commented May 26, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented May 26, 2022

saratvemulapalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 13, 2022

owaiskazi19 left a comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 22, 2022

saratvemulapalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 23, 2022

opensearch-ci-bot commented Jun 24, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 24, 2022

dbwiddis Jun 6, 2022 •

edited

Loading