[Feature/extensions] Added dependency information to Extensions #5438

mloufra · 2022-12-02T19:33:35Z

Signed-off-by: Frank Lou mloufra@amazon.com

Description

Create ExtensionDependency class with two field (uniqueId and version) to get and store dependency information data
Add optional field List<ExtensionDependency> dependencies in DiscoveryExtension class to let Jackson ObjectMapper can automatically map the dependencies entry to List<ExtensionDependency>.
Update the extensions.yml to show how it looks like after get the dependency information.

Issues Resolved

Fixs opensearch-project/opensearch-sdk-java#151

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 · 2022-12-02T19:41:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7553/
CommitID: f031ae6
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-05T17:14:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7603/
CommitID: 6de8201
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis

Good start. Don't forget you'll need tests.

dbwiddis · 2022-12-05T18:43:42Z

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

    }

    @Override
    public void writeTo(StreamOutput out) throws IOException {
        super.writeTo(out);
        pluginInfo.writeTo(out);
+        ((DiscoveryNode) dependencies).writeTo(out);


Dependencies is a List, you shouldn't cast it to another class. I believe you either want out.writeList(List<? extends Writeable> list) here, or you want to write an integer of the size and then write each list element. See example here

dbwiddis · 2022-12-05T18:54:12Z

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

@@ -61,6 +67,7 @@ public void writeTo(StreamOutput out) throws IOException {
    public DiscoveryExtension(final StreamInput in) throws IOException {
        super(in);
        this.pluginInfo = new PluginInfo(in);
+        this.dependencies = new ArrayList<ExtensionDenpendency>();


You need the counterpart here of whatever you're writing in writeTo() above. Simplest approach is probably to read the list size, declare your array list with that size, and then iterate up to that size and add to it. See example here

There's another option of in.readList() that involves using a Reader but a more simple iteration will probably work here.

dbwiddis · 2022-12-05T18:56:30Z

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

+import org.opensearch.common.io.stream.Writeable;
+
+public class ExtensionDenpendency implements Writeable {
+    public String uniqueId;


make these private since we have getters for them.

dbwiddis · 2022-12-05T18:57:11Z

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

+    public ExtensionDenpendency(String uniqueId, Version version){
+        this.uniqueId = uniqueId;
+        this.version = version;
+        List<ExtensionDenpendency> dependencies = Collections.<ExtensionDenpendency>emptyList();


No need to add a generic type for Collections.emptyList(). It is inferred from the type definition.

dbwiddis · 2022-12-05T18:59:14Z

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

@@ -333,7 +333,8 @@ private void extensionsDiscovery() throws IOException {
                                extension.getClassName(),
                                new ArrayList<String>(),
                                Boolean.parseBoolean(extension.hasNativeController())
-                            )
+                            ),
+                            new ArrayList<ExtensionDenpendency>()


shouldn't this be extension.getDependencies()?

github-actions · 2022-12-06T00:13:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7620/
CommitID: 1ae4284
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-06T00:16:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7621/
CommitID: 9cba4e3
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

mloufra · 2022-12-06T00:26:31Z

Good start. Don't forget you'll need tests.

for the tests, do you means ExtensionsOrchestratorTest.java or I need to create a new test for ExtensionDependency.java?

github-actions · 2022-12-06T17:15:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7660/
CommitID: 3fe9215
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-06T17:29:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7663/
CommitID: 4a59215
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-06T17:31:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7664/
CommitID: c116a45
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-06T17:44:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7665/
CommitID: 268fd60
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-07T00:51:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7713/
CommitID: 1b7801f
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-07T01:04:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7714/
CommitID: 9aaaeb1
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-07T01:27:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7715/
CommitID: f550c57
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis

This is looking really good! A few more tests needed!

dbwiddis · 2022-12-07T02:41:41Z

CHANGELOG.md

@@ -128,6 +128,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Enforce type safety for RegisterTransportActionsRequest([#4796](https://github.com/opensearch-project/OpenSearch/pull/4796))
 - Modified local node request to return Discovery Node ([#4862](https://github.com/opensearch-project/OpenSearch/pull/4862))
 - Enforce type safety for NamedWriteableRegistryParseRequest ([#4923](https://github.com/opensearch-project/OpenSearch/pull/4923))
+ - Update dependency information ([#5438](https://github.com/opensearch-project/OpenSearch/pull/5438))


(Preference) Probably want this change log (and PR title) to be a little bit more self-explanatory, like "Add dependency information to Extensions"

dbwiddis · 2022-12-07T02:44:19Z

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

    }

    @Override
    public void writeTo(StreamOutput out) throws IOException {
        super.writeTo(out);
        pluginInfo.writeTo(out);
+        out.writeVInt(dependencies.size());
+        for (ExtensionDependency result : dependencies) {


(Preference) Let's use a loop variable name representing what we actually have here, like dependency or dep.

dbwiddis · 2022-12-07T02:52:00Z

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

@@ -43,6 +43,7 @@ public static class Extension {
        private String className;
        private String customFolderName;
        private String hasNativeController;
+        private List<ExtensionDependency> dependencies;


I'm not sure what Jackson's ObjectMapper does reading in a list from a YAML file when it doesn't exist. (If an extension has no dependencies we may want to allow them to leave dependencies: blank. It may end up null.

Since Jackson uses the no-arg constructor right below this, I'd suggest you initialize this to an empty list in that no-arg constructor block.

Rereading this again, I see this is static inner Extension class here in ExtensionSettings. I don't think this is ever used or is needed at all. It's completely redundant with the DiscoveryExtension class which is used elsewhere.

The change to add the list of dependencies is done in the DiscoveryExtension class.

I actually think this entire inner class can be deleted. @owaiskazi19 do you agree?

@dbwiddis Can we use Arrays.asList(new ExtensionDependency(extension.getUniqueId(), Version.fromString(extension.getOpensearchVersion()))) to replace extension.getDepdendency in ExtensionsOrchestrator, in this way, we can delete the getDependency method in ExtensionSetting.

@dbwiddis Can we use Arrays.asList(...

It's been a while since I did YAML import from Jackson. If Jackson reads in a list as an array, then:

define the dependencies in this class as an array

in ExtensionsOrchestrator, convert the array to a list as Arrays.asList(extension.getDependencies())

dbwiddis · 2022-12-07T02:52:20Z

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

+            return dependencies;
+        }
+
+        public void setDependencies(List<ExtensionDependency> dependencies) {


I don't think we need a setter for this list.

dbwiddis · 2022-12-07T02:53:39Z

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

@@ -184,6 +185,14 @@ public void setHasNativeController(String hasNativeController) {
            this.hasNativeController = hasNativeController;
        }

+        public List<ExtensionDependency> getDependenices() {


Typo in the getter name here.

dbwiddis · 2022-12-07T02:58:51Z

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

            )
        );
        assertEquals(expectedExtensionsList.size(), extensionsOrchestrator.extensionIdMap.values().size());
        assertTrue(expectedExtensionsList.containsAll(extensionsOrchestrator.extensionIdMap.values()));
        assertTrue(extensionsOrchestrator.extensionIdMap.values().containsAll(expectedExtensionsList));
    }

+    public void testExtensionDependency() throws Exception {


Great job on this test method for the new Dependency class.

You also need to alter the test for the DiscoveryExtension to test the new dependencies field.

Also higher up in the class, see where the YAML file is generated using a list of strings. You should add a dependency (or two) in the list for the second item (same as the test yaml file below), so you can:

test that the first extension's dependency list is empty

test that the second dependency list has an item with the appropriate unique ID and version.

github-actions · 2022-12-07T22:55:16Z

Gradle Check (Jenkins) Run Completed with:

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

codecov-commenter · 2022-12-07T22:57:33Z

Codecov Report

Merging #5438 (ea6c97c) into feature/extensions (b49e2af) will increase coverage by 0.12%.
The diff coverage is 80.00%.

@@                   Coverage Diff                    @@
##             feature/extensions    #5438      +/-   ##
========================================================
+ Coverage                 70.97%   71.09%   +0.12%     
- Complexity                58486    58541      +55     
========================================================
  Files                      4756     4757       +1     
  Lines                    278926   278956      +30     
  Branches                  40283    40287       +4     
========================================================
+ Hits                     197964   198321     +357     
+ Misses                    64810    64516     -294     
+ Partials                  16152    16119      -33

Impacted Files	Coverage Δ
...org/opensearch/extensions/ExtensionDependency.java	`68.42% <68.42%> (ø)`
.../org/opensearch/extensions/DiscoveryExtension.java	`94.73% <100.00%> (+4.73%)`	⬆️
.../opensearch/extensions/ExtensionsOrchestrator.java	`66.84% <100.00%> (+0.18%)`	⬆️
.../org/opensearch/extensions/ExtensionsSettings.java	`96.61% <100.00%> (+0.05%)`	⬆️
...cluster/coordination/PendingClusterStateStats.java	`20.00% <0.00%> (-48.00%)`	⬇️
...pensearch/action/ingest/DeletePipelineRequest.java	`31.25% <0.00%> (-37.50%)`	⬇️
...ion/admin/cluster/node/info/PluginsAndModules.java	`53.12% <0.00%> (-34.38%)`	⬇️
...search/search/aggregations/pipeline/EwmaModel.java	`24.44% <0.00%> (-33.34%)`	⬇️
...search/aggregations/pipeline/HoltWintersModel.java	`21.47% <0.00%> (-29.54%)`	⬇️
...opensearch/index/reindex/BulkByScrollResponse.java	`48.38% <0.00%> (-28.23%)`	⬇️
... and 457 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-12-08T00:24:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7770/
CommitID: 533e653
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis · 2022-12-08T00:26:34Z

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

            )
        );
        assertEquals(expectedExtensionsList.size(), extensionsOrchestrator.extensionIdMap.values().size());
        assertTrue(expectedExtensionsList.containsAll(extensionsOrchestrator.extensionIdMap.values()));
        assertTrue(extensionsOrchestrator.extensionIdMap.values().containsAll(expectedExtensionsList));
+        assertEquals(expectedExtensionsList.get(expectedExtensionsList.size() - 1), dependency.get(0));


This works, but since we know we created a single-dependency list, can we just get(0) here?

You are right. we do not need the get(0), because we already create dependency as a single list and it is all we need.

You are right. we do not need the get(0), because we already create dependency as a single list and it is all we need.

Correct. We could even ignore the list and actually create a expectedDependency object.

github-actions · 2022-12-08T00:42:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7772/
CommitID: 3818817
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-08T01:02:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7773/
CommitID: e4d81dc
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-08T18:21:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7799/
CommitID: 4bb930b
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-08T18:39:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7802/
CommitID: 0c539f1
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-13T18:49:01Z

Gradle Check (Jenkins) Run Completed with:

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

mloufra requested a review from owaiskazi19 December 2, 2022 19:38

mloufra requested a review from dbwiddis December 5, 2022 17:06

dbwiddis requested changes Dec 5, 2022

View reviewed changes

mloufra requested a review from dbwiddis December 7, 2022 01:30

dbwiddis requested changes Dec 7, 2022

View reviewed changes

mloufra changed the title ~~[Feature/extensions] Update dependency information~~ [Feature/extensions] Add dependency information to Extensions Dec 7, 2022

mloufra marked this pull request as ready for review December 7, 2022 17:59

mloufra requested review from a team and reta as code owners December 7, 2022 17:59

mloufra force-pushed the update-dependency-information branch from f550c57 to ea6c97c Compare December 7, 2022 22:25

dbwiddis reviewed Dec 8, 2022

View reviewed changes

mloufra requested review from nknize, adnapibar, Rishikesh1159, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners December 13, 2022 18:12

mloufra removed request for dblock, nknize, mch2, adnapibar, shwetathareja, VachaShah, andrross, Bukhtawar, saratvemulapalli, kotwanikunal, anasalkouz, setiah, Rishikesh1159, CEHENKLE, tlfeng, dreamer-89, kartg and xuezhou25 December 13, 2022 18:13

owaiskazi19 merged commit 895c2ae into opensearch-project:feature/extensions Dec 13, 2022

mloufra deleted the update-dependency-information branch December 13, 2022 19:07

[Feature/extensions] Added dependency information to Extensions #5438

[Feature/extensions] Added dependency information to Extensions #5438

Conversation

mloufra commented Dec 2, 2022

Description

Issues Resolved

Check List

github-actions bot commented Dec 2, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 5, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

mloufra commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 7, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 7, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 7, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis 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

Choose a reason for hiding this comment

mloufra Dec 8, 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

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Dec 7, 2022

Codecov Report

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 13, 2022

Gradle Check (Jenkins) Run Completed with:

mloufra Dec 8, 2022 •

edited

Loading