[Feature/extensions] Adding Transport Actions support for extensions by saratvemulapalli · Pull Request #4598 · opensearch-project/OpenSearch

saratvemulapalli · 2022-09-26T17:39:17Z

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

Description

Adding transport actions support for extensions via ExtensionMainAction as proxy.

To help understand the workflow:

ExtensionsOrchestrator will register the list of actions with one proxy action for all extensions ExtensionMainAction and register it with ActionModule.
Extensions make register transport actions[1] and ExtensionActions will manage all actions for all extensions.
ExtensionsOrchestrator will respond with Ack to the extension after a successful registration of the action.
When a transport request comes along,ExtensionOrchestrator will look up the registry and make a transport action call to the node which will eventually end up on ExtensionsOrchestrator as it registers ExtensionMainAction.
ExtensionsOrchestrator will make a transport API call to the extension to process the request.
After the response comes back, ExtensionsOrchestrator will translate the response and send it back to the extension which made the request.

[1] https://github.com/opensearch-project/OpenSearch/blob/feature/extensions/server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java#L253

Issues Resolved

opensearch-project/opensearch-sdk-java#106

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-26T17:59:35Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-09-26T18:00:08Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-09-26T18:00:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3424/
CommitID: 1e60cf0

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

github-actions · 2022-09-26T18:34:32Z

Gradle Check (Jenkins) Run Completed with:

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

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

github-actions · 2022-09-27T01:24:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3445/
CommitID: 201d7be

github-actions · 2022-09-27T01:29:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3446/
CommitID: 9fd0fd2

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

github-actions · 2022-09-27T02:46:03Z

Gradle Check (Jenkins) Run Completed with:

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

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

github-actions · 2022-09-27T03:44:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/3448/
CommitID: 8fb2911

codecov-commenter · 2022-09-27T03:45:38Z

Codecov Report

Merging #4598 (02841db) into feature/extensions (54b4ad0) will increase coverage by 0.02%.
The diff coverage is 56.58%.

@@                   Coverage Diff                    @@
##             feature/extensions    #4598      +/-   ##
========================================================
+ Coverage                 70.63%   70.65%   +0.02%     
- Complexity                57533    57950     +417     
========================================================
  Files                      4656     4708      +52     
  Lines                    276711   278528    +1817     
  Branches                  40437    40661     +224     
========================================================
+ Hits                     195457   196805    +1348     
- Misses                    64833    65233     +400     
- Partials                  16421    16490      +69

Impacted Files	Coverage Δ
.../java/org/opensearch/client/RequestConverters.java	`85.85% <0.00%> (+1.02%)`	⬆️
...ava/org/opensearch/client/RestHighLevelClient.java	`41.75% <0.00%> (-0.66%)`	⬇️
...va/org/opensearch/geometry/GeometryCollection.java	`92.85% <0.00%> (-7.15%)`	⬇️
...le/customsettings/ExampleCustomSettingsConfig.java	`29.03% <ø> (ø)`
...le/customsettings/ExampleCustomSettingsPlugin.java	`9.09% <ø> (ø)`
...igheuristic/CustomSignificanceHeuristicPlugin.java	`0.00% <0.00%> (ø)`
...ch/example/customsigheuristic/SimpleHeuristic.java	`86.66% <ø> (ø)`
...earch/example/customsuggester/CustomSuggester.java	`0.00% <0.00%> (ø)`
...example/customsuggester/CustomSuggesterPlugin.java	`0.00% <0.00%> (ø)`
...arch/example/customsuggester/CustomSuggestion.java	`0.00% <ø> (ø)`
... and 647 more

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

dbwiddis

Some initial thoughts. Doesn't seem like most of the new code has any tests for it, unless I'm missing something in another PR.

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

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

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

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

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

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

dbwiddis · 2022-09-27T04:28:28Z

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

+/**
+ * This class encapsulates a transport request to extension
+ *
+ * @opensearch.api


Not necessarily in this PR but someone should open an issue to discuss what extension classes should be api vs. internal. I'm not so sure about this one.

So this is a transport request going to an extension. The extension would use it as an API to understand the request.
But sure let me open up and issue for this, I agree we need a little more wider discussion and alignment.

Internals classes are termed as internal which can't be extended. Classes which can be extended are termed as api

Agreed, but are we expecting users to extend this class or is it here for our convenience?

Not really extend but we expect extensions to use it. Anyway I'll open up an issue for this.
opensearch-project/opensearch-sdk-java#168

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

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

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

github-actions · 2022-09-27T18:37:20Z

Gradle Check (Jenkins) Run Completed with:

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

owaiskazi19 · 2022-09-27T21:47:51Z

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

        registerRequestHandler();
    }

+    public ExtensionActionResponse handleTransportRequest(ExtensionActionRequest request) throws InterruptedException {


handleActionRequest?

Sure, if you like that better.

I like handleTransportAction to differentiate it from handleRestAction etc.

I'll leave it as handleTransportAction for now. @owaiskazi19 let us know if you strongly disagree :)

saratvemulapalli · 2022-09-27T21:54:56Z

Some initial thoughts. Doesn't seem like most of the new code has any tests for it, unless I'm missing something in another PR.

You are right. Added all the tests :) Thanks for the review.

owaiskazi19 · 2022-09-30T22:59:37Z

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

+     * @param transportActionsRequest  The request to handle.
+     * @return  A {@link ExtensionBooleanResponse} indicating success.
+     */
+    public TransportResponse handleRegisterTransportActionsRequest(RegisterTransportActionsRequest transportActionsRequest) {


Separate handler class here?

Its just one single method, I'd prefer to have all the functionality with the same class as it's taking care of transport actions. If the handlers grow to multiple of them we can start splitting them.

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

owaiskazi19 · 2022-09-30T23:02:36Z

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

+
+                @Override
+                public void onFailure(Exception exp) {
+                    logger.debug("Transport request failed", exp);


Suggested change

logger.debug("Transport request failed", exp);

logger.debug("Transport Action request failed", exp);

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

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

owaiskazi19 · 2022-09-30T23:17:35Z

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

+ *
+ * @opensearch.internal
+ */
+public class ExtensionTransportAction extends HandledTransportAction<ExtensionActionRequest, ExtensionActionResponse> {


This is really great. We are able to send request from one extension to another. Just wanted to know how did you test it @saratvemulapalli manually? Did you register actions of 2 different extensions?

Yup.

registered an action on extension

hook up the Rest Handler API in the extension to make a transport request to the same action

Expect the orchestrator to get the request, proxy it back to the action

Extension responds to the transport action request

Orchestrator gets the response, proxy it back to the caller

Extension receives the response.

I can paste the logs, but its huge :D

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

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

github-actions · 2022-10-06T18:22:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3996/
CommitID: 8bdb469

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

github-actions · 2022-10-06T21:22:39Z

Gradle Check (Jenkins) Run Completed with:

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

saratvemulapalli · 2022-10-06T22:12:22Z

Looks like failures from updating lucene snapshots.
Will rebase main with feature/extensions and my PR as well.

[org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant](https://build.ci.opensearch.org/job/gradle-check/4048/testReport/junit/org.opensearch.qa.verify_version_constants/VerifyVersionConstantsIT/testLuceneVersionConstant/)
[org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant](https://build.ci.opensearch.org/job/gradle-check/4048/testReport/junit/org.opensearch.qa.verify_version_constants/VerifyVersionConstantsIT/testLuceneVersionConstant_2/)
[org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant](https://build.ci.opensearch.org/job/gradle-check/4048/testReport/junit/org.opensearch.qa.verify_version_constants/VerifyVersionConstantsIT/testLuceneVersionConstant_3/)
[org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant](https://build.ci.opensearch.org/job/gradle-check/4048/testReport/junit/org.opensearch.qa.verify_version_constants/VerifyVersionConstantsIT/testLuceneVersionConstant_4/)

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

github-actions · 2022-10-06T23:50:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4069/
CommitID: 6b59cd0

dbwiddis

Approved but have made some suggestions I hope you'll consider.

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

dbwiddis · 2022-10-07T18:24:52Z

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

+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) return true;
+        if (obj == null || getClass() != obj.getClass()) return false;


(Suggestion) my IDE tells me to prefer instanceof to getClass() equality. I don't know why. OK as-is, of course.

(Suggestion) my IDE tells me to prefer instanceof to getClass() equality. I don't know why. OK as-is, of course.

@saratvemulapalli So I actually looked up why. It's related to subclassing. If you had a class Foo extends Bar and then wanted to use Bar with anything requiring equals (e.g., hashmap key) and you had two Foo objects that were equal in all other ways but did not override equals themselves, they wouldn't be equal.

My IDE has a checkbox for the equals code generator to switch it to instanceof.

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

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

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

...src/test/java/org/opensearch/extensions/action/TransportActionRequestFromExtensionTests.java

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

github-actions · 2022-10-07T21:27:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4180/
CommitID: 02841db

dbwiddis

LGTM!

github-actions · 2022-10-07T22:10:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4189/
CommitID: 02841db

github-actions · 2022-10-07T22:40:18Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4192/
CommitID: 02841db

Adding Transport Actions support for extensions

a4c164e

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

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

saratvemulapalli added 2 commits September 26, 2022 10:40

Merge branch 'feature/extensions' into proxy-actions

1e60cf0

Adding changelog

e1e3b02

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

Fixing precommit and tests

5bc91ed

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

saratvemulapalli added 4 commits September 26, 2022 17:10

Adding Transport Actions support for extensions

3dc6f53

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

Merge branch 'feature/extensions' into proxy-actions

2bb61d1

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

Fixing tests

201d7be

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

Close client which is leaking threads

9fd0fd2

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

Fixing spotless

a82bffa

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

Few more tests

8fb2911

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

dbwiddis reviewed Sep 27, 2022

View reviewed changes

Adding tests

f7747a1

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

owaiskazi19 reviewed Sep 27, 2022

View reviewed changes

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java Show resolved Hide resolved

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

server/src/main/java/org/opensearch/extensions/action/ExtensionMainAction.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Sep 30, 2022

View reviewed changes

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

saratvemulapalli mentioned this pull request Oct 4, 2022

Adding support for extension to register transport actions opensearch-project/opensearch-sdk-java#164

Merged

saratvemulapalli added 2 commits October 5, 2022 14:58

Merge branch 'feature/extensions' into proxy-actions

1dc24ee

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

Addressing comments

8bdb469

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

Addressing comments and fixing spotless

ba8b56d

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

saratvemulapalli requested a review from dbwiddis October 6, 2022 20:59

Merge branch 'feature/extensions' into proxy-actions

6b59cd0

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

saratvemulapalli requested a review from owaiskazi19 October 7, 2022 00:05

dbwiddis approved these changes Oct 7, 2022

View reviewed changes

Addressing comments

02841db

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

saratvemulapalli requested a review from dbwiddis October 7, 2022 21:02

dbwiddis approved these changes Oct 7, 2022

View reviewed changes

saratvemulapalli merged commit eee59c5 into opensearch-project:feature/extensions Oct 7, 2022

saratvemulapalli deleted the proxy-actions branch October 7, 2022 23:24

This was referenced Oct 10, 2022

[FEATURE] Refactor ExtensionsRunner test code out of production source tree opensearch-project/opensearch-sdk-java#171

Closed

[BUG] RegisterTransportActionsRequest constructor requires two arguments opensearch-project/opensearch-sdk-java#173

Closed

ryanbogan mentioned this pull request Nov 17, 2022

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

Closed

	logger.debug("Transport request failed", exp);
	logger.debug("Transport Action request failed", exp);

Conversation

saratvemulapalli commented Sep 26, 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 Sep 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

github-actions bot commented Sep 27, 2022

Gradle Check (Jenkins) Run Completed with:

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 Oct 6, 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!

saratvemulapalli commented Sep 27, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

saratvemulapalli commented Sep 26, 2022 •

edited

Loading

codecov-commenter commented Sep 27, 2022 •

edited

Loading

saratvemulapalli Oct 6, 2022 •

edited

Loading

saratvemulapalli commented Sep 27, 2022 •

edited

Loading