-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Feature/extensions] Adding Transport Actions support for extensions #4598
[Feature/extensions] Adding Transport Actions support for extensions #4598
Conversation
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/RegisterTransportActionsRequest.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/ExtensionActionResponse.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/ExtensionsOrchestrator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java
Outdated
Show resolved
Hide resolved
/** | ||
* This class encapsulates a transport request to extension | ||
* | ||
* @opensearch.api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internals classes are termed as internal
which can't be extended. Classes which can be extended are termed as api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but are we expecting users to extend this class or is it here for our convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/TransportActionRequestFromExtension.java
Show resolved
Hide resolved
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
registerRequestHandler(); | ||
} | ||
|
||
public ExtensionActionResponse handleTransportRequest(ExtensionActionRequest request) throws InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleActionRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if you like that better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like handleTransportAction
to differentiate it from handleRestAction
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it as handleTransportAction
for now. @owaiskazi19 let us know if you strongly disagree :)
You are right. Added all the tests :) Thanks for the review. |
* @param transportActionsRequest The request to handle. | ||
* @return A {@link ExtensionBooleanResponse} indicating success. | ||
*/ | ||
public TransportResponse handleRegisterTransportActionsRequest(RegisterTransportActionsRequest transportActionsRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate handler class here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void onFailure(Exception exp) { | ||
logger.debug("Transport request failed", exp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.debug("Transport request failed", exp); | |
logger.debug("Transport Action request failed", exp); |
server/src/main/java/org/opensearch/extensions/action/ExtensionActions.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/extensions/action/ExtensionMainAction.java
Outdated
Show resolved
Hide resolved
* | ||
* @opensearch.internal | ||
*/ | ||
public class ExtensionTransportAction extends HandledTransportAction<ExtensionActionRequest, ExtensionActionResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Looks like failures from updating lucene snapshots.
|
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but have made some suggestions I hope you'll consider.
server/src/main/java/org/opensearch/extensions/action/ExtensionProxyAction.java
Show resolved
Hide resolved
@Override | ||
public boolean equals(Object obj) { | ||
if (this == obj) return true; | ||
if (obj == null || getClass() != obj.getClass()) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Suggestion) my IDE tells me to prefer instanceof
to getClass()
equality. I don't know why. OK as-is, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Suggestion) my IDE tells me to prefer
instanceof
togetClass()
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
Outdated
Show resolved
Hide resolved
server/src/test/java/org/opensearch/extensions/action/ExtensionActionRequestTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/opensearch/extensions/action/ExtensionActionResponseTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/opensearch/extensions/action/ExtensionHandleTransportRequestTests.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/opensearch/extensions/action/TransportActionRequestFromExtensionTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
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 extensionsExtensionMainAction
and register it withActionModule
.ExtensionActions
will manage all actions for all extensions.ExtensionsOrchestrator
will respond with Ack to the extension after a successful registration of the action.ExtensionOrchestrator
will look up the registry and make a transport action call to the node which will eventually end up on ExtensionsOrchestrator as it registersExtensionMainAction
.ExtensionsOrchestrator
will make a transport API call to the extension to process the request.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
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.