Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute remote actions on another extension #588

Merged
merged 21 commits into from
Mar 24, 2023

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Mar 20, 2023

Companion PR: #6734 must be merged first

Description

Allows one extension to execute an action on another extension.

See the Hello World RestRemoteHelloAction for sample implementation.

Other work done to support:

  • Put a setter on Extension interface to get a copy of extensions runner since injection is flaky.
  • Created SDKTransportService class to avoid having to pass around so many params. Lots of things can be moved from ExtensionsRunner there later, just did the action module ones in this PR and left some TODOs where other opportunities exist

Issues Resolved

Fixes #525

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.

@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 20, 2023

Gradle check is failing based on companion PR classes. No idea why DCO check is failing, git log shows they're all signed. (git rebase --signoff HEAD~9 for the win!)

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on Dynamic Actions @dbwiddis 🙌! Didn't get a chance to review the tests

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Copy link
Member Author

@owaiskazi19 @saratvemulapalli addressed all your comments.

And as-of now it still works:

curl -X GET http://localhost:9200/_extensions/_hw/hello/test
Received greeting from remote extension: Hello, test%  

@dbwiddis
Copy link
Member Author

Gradle check failing due to companion PR. Only thing left is to update the sequence diagram which I'm working on.

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@joshpalis joshpalis self-requested a review March 23, 2023 23:12
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for incorporating all the suggestions!

Copy link
Member

@joshpalis joshpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @dbwiddis for this.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@467200d). Click here to learn what that means.
The diff coverage is 42.27%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #588   +/-   ##
=======================================
  Coverage        ?   63.97%           
  Complexity      ?      244           
=======================================
  Files           ?       49           
  Lines           ?     1088           
  Branches        ?       35           
=======================================
  Hits            ?      696           
  Misses          ?      381           
  Partials        ?       11           
Impacted Files Coverage Δ
src/main/java/org/opensearch/sdk/Extension.java 100.00% <ø> (ø)
...h/sdk/handlers/ExtensionActionResponseHandler.java 0.00% <0.00%> (ø)
...ch/sdk/handlers/ExtensionActionRequestHandler.java 7.27% <7.27%> (ø)
.../sample/helloworld/rest/RestRemoteHelloAction.java 25.00% <25.00%> (ø)
...rch/sdk/action/RemoteExtensionTransportAction.java 37.50% <37.50%> (ø)
...n/java/org/opensearch/sdk/SDKTransportService.java 47.61% <47.61%> (ø)
...earch/sdk/action/RemoteExtensionActionRequest.java 71.79% <71.79%> (ø)
...main/java/org/opensearch/sdk/ExtensionsRunner.java 76.96% <78.57%> (ø)
...rc/main/java/org/opensearch/sdk/BaseExtension.java 86.66% <100.00%> (ø)
src/main/java/org/opensearch/sdk/SDKClient.java 92.00% <100.00%> (ø)
... and 4 more

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

@dbwiddis dbwiddis merged commit ebc684a into opensearch-project:main Mar 24, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 24, 2023
* Add ProxyAction with TransportAction and handlers

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Give SDKActionModule a copy of ExtensionsRunner to use with transport

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add new ProxyActionRequest

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add SDKTransportService wrapper accessible to actions

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Implement ProxyTransportAction

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add test case to HelloWorldExtension

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Better naming of ExtensionActionResponse and correct action name

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Refactoring with TransportService and latest OpenSearch PR updates

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add ExtensionsActionRequestHandler

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Instantiate Proxy Action Request

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Working test case!

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Properly parse returned byte array into a response

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add sequence diagram to DESIGN.md

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Typoo fix

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Update with latest changes on companion PR

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Rename ProxyFoo to RemoteExtensionFoo

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Better handling of response bytes

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Handle plugin remote action requests

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Address code review comments

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Update sequence diagram

Signed-off-by: Daniel Widdis <widdis@gmail.com>

---------

Signed-off-by: Daniel Widdis <widdis@gmail.com>
(cherry picked from commit ebc684a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@dbwiddis dbwiddis deleted the dynamicActions branch March 24, 2023 19:41
owaiskazi19 pushed a commit that referenced this pull request Mar 30, 2023
* Add ProxyAction with TransportAction and handlers



* Give SDKActionModule a copy of ExtensionsRunner to use with transport



* Add new ProxyActionRequest



* Add SDKTransportService wrapper accessible to actions



* Implement ProxyTransportAction



* Add test case to HelloWorldExtension



* Better naming of ExtensionActionResponse and correct action name



* Refactoring with TransportService and latest OpenSearch PR updates



* Add ExtensionsActionRequestHandler



* Instantiate Proxy Action Request



* Working test case!



* Properly parse returned byte array into a response



* Add sequence diagram to DESIGN.md



* Typoo fix



* Update with latest changes on companion PR



* Rename ProxyFoo to RemoteExtensionFoo



* Better handling of response bytes



* Handle plugin remote action requests



* Address code review comments



* Update sequence diagram



---------


(cherry picked from commit ebc684a)

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joshua Palis <jpalis@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] [DISCUSS] Implement Proxy Actions
6 participants