[Feature/extensions] First draft of adding support for dynamically registering actions by saratvemulapalli · Pull Request #4460 · opensearch-project/OpenSearch

saratvemulapalli · 2022-09-08T17:51:34Z

Description

With extensions, we would like to register actions during runtime of OpenSearch and not only during bootstrap.
This PR enables ActionModule in OpenSearch core to register transport actions dynamically.

Issues Resolved

opensearch-project/opensearch-sdk-java#107

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.

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2022-09-08T17:52:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: c1301cd

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2022-09-12T20:13:13Z

Gradle Check (Jenkins) Run Completed with:

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

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

saratvemulapalli · 2022-09-13T16:39:28Z

ChangeLog action failing, I'll wait for #4479 to merge and then rebase. Because it will eventually end up in conflicts.

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2022-09-13T16:52:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3013/
CommitID: 329ce48

github-actions · 2022-09-13T16:54:36Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-09-13T16:55:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3010/
CommitID: 57d6e84

github-actions · 2022-09-13T16:56:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3011/
CommitID: 540fa4b

github-actions · 2022-09-13T16:59:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3012/
CommitID: 5f6bc96

saratvemulapalli · 2022-09-13T17:09:28Z

1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':server:spotlessJavaCheck'.
> The following files had format violations:
      src/main/java/org/opensearch/node/Node.java
          @@ -1022,7 +1022,8 @@
           ············resourcesToClose.add(injector.getInstance(PeerRecoverySourceService.class));
           ············this.pluginLifecycleComponents·=·Collections.unmodifiableList(pluginLifecycleComponents);
           ············client.initialize(injector.getInstance(new·Key<Map<ActionType,·TransportAction>>()·{
          -················}),·injector.getInstance(ActionRegistry.class),
          +············}),
          +················injector.getInstance(ActionRegistry.class),
           ················()·->·clusterService.localNode().getId(),
           ················transportService.getRemoteClusterService(),
           ················namedWriteableRegistry
      src/test/java/org/opensearch/client/node/NodeClientHeadersTests.java
          @@ -32,7 +32,6 @@
           
           package·org.opensearch.client.node;
           
          -import·org.opensearch.action.ActionModule;
           import·org.opensearch.action.ActionType;
           import·org.opensearch.action.ActionListener;
           import·org.opensearch.action.ActionRequest;
      src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java
          @@ -38,7 +38,6 @@
           import·org.opensearch.ExceptionsHelper;
           import·org.opensearch.Version;
           import·org.opensearch.action.ActionListener;
          -import·org.opensearch.action.ActionModule;
           import·org.opensearch.action.ActionType;
           import·org.opensearch.action.RequestValidators;
           import·org.opensearch.action.StepListener;
  Run './gradlew :server:spotlessApply' to fix these violations.

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

github-actions · 2022-09-13T17:24:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3018/
CommitID: 748a09b

saratvemulapalli · 2022-09-13T18:02:31Z

Gradle check failures:


1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':example-plugins:custom-suggester:yamlRestTest'.
> process was found dead while waiting for ports files, node{:example-plugins:custom-suggester:yamlRestTest-0}

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
==============================================================================

2: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':plugins:discovery-gce:qa:gce:yamlRestTest'.
> process was found dead while waiting for ports files, node{:plugins:discovery-gce:qa:gce:yamlRestTest-0}

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
==============================================================================

3: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':plugins:discovery-ec2:qa:amazon-ec2:yamlRestTestContainerCredentials'.
> process was found dead while waiting for ports files, node{:plugins:discovery-ec2:qa:amazon-ec2:yamlRestTestContainerCredentials-0}

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
==============================================================================

4: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':rest-api-spec:yamlRestTest'.
> process was found dead while waiting for ports files, node{:rest-api-spec:yamlRestTest-0}

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
==============================================================================

owaiskazi19 · 2022-09-15T19:12:27Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionResponse.java

+import java.io.IOException;
+
+public class ExtensionsActionResponse extends ActionResponse implements ToXContentObject {
+    String myFirstResponse;


Looks like leftover from draft PR?

owaiskazi19 · 2022-09-15T19:14:11Z

server/src/test/java/org/opensearch/rest/action/admin/indices/RestValidateQueryActionTests.java

        actions.put(ValidateQueryAction.INSTANCE, transportAction);

-        client.initialize(actions, () -> "local", null, new NamedWriteableRegistry(Collections.emptyList()));
+        client.initialize(actions, null, () -> "local", null, new NamedWriteableRegistry(Collections.emptyList()));


Can we create a dummy obj of actionsRegistry instead of passing null?

owaiskazi19 · 2022-09-15T19:15:39Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsAction.java

+import org.opensearch.action.ActionType;
+
+public class ExtensionsAction extends ActionType<ExtensionsActionResponse> {
+    public static final String NAME = "cluster:monitor/extension";


Should be keep the action name in the same format as internal...?

Sure makes sense.

owaiskazi19 · 2022-09-15T19:17:37Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionRequest.java

+import java.io.IOException;
+
+public class ExtensionsActionRequest extends ActionRequest {
+    String firstRequest;


Leftover from draft PR?

Or is this a draft PR?

Why is the first request so important to preserve?

My bad, looks like I missed a commit during the rebase.
This is moved to action which the extension would like to register.

owaiskazi19 · 2022-09-15T19:20:01Z

server/src/main/java/org/opensearch/client/node/NodeClient.java

    private <Request extends ActionRequest, Response extends ActionResponse> TransportAction<Request, Response> transportAction(
        ActionType<Response> action
    ) {
+        /* TODO add support to read actionsRegistry along with actions */


Do we have an issue to track this TODO?

Haha I've taken care of this as part of SDK#106.

owaiskazi19 · 2022-09-15T19:21:35Z

server/src/main/java/org/opensearch/node/Node.java


        final ClusterService clusterService = injector.getInstance(ClusterService.class);
+        final ActionModule actionModule = injector.getInstance(ActionModule.class);
+        actionModule.registerAction(ExtensionsAction.INSTANCE, TransportExtensionsAction.class);


Can we have tests for registerActions?

Thats a good point.
Sure.

dbwiddis

Looks generally good with comments inline. I'm confused whether this is a draft PR (per subject line) or intended to be merged. Would be helpful to have more comments indicating future functionality.

The word "Action" is overused in the codebase and at least one of these classes (probably ExtensionsAction) should describe just what we are talking about.

dbwiddis · 2022-09-16T07:35:21Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionResponse.java

+import java.io.IOException;
+
+public class ExtensionsActionResponse extends ActionResponse implements ToXContentObject {
+    String myFirstResponse;


In #4519 I merged multiple similar "string response" classes into an ExtensionStringResponse class. This looks very similar in that it's just a basic "string response" but extending ActionResponse rather than TransportResponse.

I don't think the name ExtensionsActionResponse is meaningful for the simplicity of this object.

I am also thinking we need to figure out a consistent naming convention.

I am also seeing that we are very inconsistent in pluralization of Extension in these class names.

Naming things is hard.

dbwiddis · 2022-09-16T07:39:01Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionResponse.java

+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeString(myFirstResponse);


I haven't dug into the implementation details, but when I see a super(in) I expect to see a matching super(out). I know the super is required for transport requests but not responses, and in the latter case the super call is a noop.

All that to say, either add a super(out) here or delete the super(in) above.

dbwiddis · 2022-09-16T07:40:10Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionResponse.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        return null;


What will eventually go here? I don't like seeing null returns, but a comment suggesting future usage would make me happier.

dbwiddis · 2022-09-16T07:42:39Z

server/src/test/java/org/opensearch/extensions/action/ExtensionsActionTests.java

+
+public class ExtensionsActionTests extends OpenSearchSingleNodeTestCase {
+
+    public void testExtensionAction() throws ExecutionException, InterruptedException {


usually tests contain assertions of some sort. :)

Yup I just realized I missed a commit :D

dbwiddis · 2022-09-16T07:44:16Z

server/src/main/java/org/opensearch/extensions/action/ExtensionsActionRequest.java

+import java.io.IOException;
+
+public class ExtensionsActionRequest extends ActionRequest {
+    String firstRequest;


Or is this a draft PR?

Why is the first request so important to preserve?

dbwiddis · 2022-09-16T07:47:21Z

server/src/main/java/org/opensearch/action/ActionModule.java

    private final SettingsFilter settingsFilter;
    private final List<ActionPlugin> actionPlugins;
    private final Map<String, ActionHandler<?, ?>> actions;
+    private ActionRegistry actionRegistryMap;


Everything around this is final. Should this also be?

Thats a good point. I was debating but it can be a final because the instance would not change.
Will update.

saratvemulapalli · 2022-09-22T17:42:36Z

Thanks for the feedback @dbwiddis, @owaiskazi19 !
Conflicts causing problems. Im sending out a clean PR and closing this one.

Adding support for dynamically registering Actions

c1301cd

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

saratvemulapalli requested review from a team and reta as code owners September 8, 2022 17:51

saratvemulapalli marked this pull request as draft September 8, 2022 17:57

saratvemulapalli added 2 commits September 12, 2022 11:07

Adding support for dynamically registering Actions

f2ceada

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Merge branch 'feature/extensions' into dynamic-actions

7aba611

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Rebasing with feature/extensions

57d6e84

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

saratvemulapalli marked this pull request as ready for review September 13, 2022 16:32

saratvemulapalli added 2 commits September 13, 2022 09:33

Removing commented code

540fa4b

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Removing debug configuration

5f6bc96

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

saratvemulapalli added 2 commits September 13, 2022 09:40

Fixing typo

329ce48

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Another typo

2999239

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

Fixing spotless

748a09b

Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>

owaiskazi19 reviewed Sep 15, 2022

View reviewed changes

owaiskazi19 requested a review from dbwiddis September 15, 2022 19:22

dbwiddis reviewed Sep 16, 2022

View reviewed changes

saratvemulapalli closed this Sep 22, 2022

dbwiddis mentioned this pull request Mar 17, 2023

[Extensions] Add DynamicActionRegistry to ActionModule #6734

Merged

5 tasks


		public class ExtensionsActionTests extends OpenSearchSingleNodeTestCase {

		public void testExtensionAction() throws ExecutionException, InterruptedException {

Conversation

saratvemulapalli commented Sep 8, 2022

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Sep 8, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 12, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

saratvemulapalli commented Sep 13, 2022

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

saratvemulapalli commented Sep 13, 2022

Uh oh!

github-actions bot commented Sep 13, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

saratvemulapalli commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Sep 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Sep 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli commented Sep 13, 2022 •

edited

Loading

owaiskazi19 Sep 15, 2022 •

edited

Loading

owaiskazi19 Sep 15, 2022 •

edited

Loading