-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Extensions] Add DynamicActionRegistry to ActionModule #6734
Conversation
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.
Nice! I like the steps forward to consolidating all actions together. I've put in some notes about coupling and how the abstraction could work a little differently that I think could be more flexible.
server/src/main/java/org/opensearch/client/node/NodeClient.java
Outdated
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
} | ||
actionsMap.putIfAbsent(action, extension); | ||
// Register the action in the action module's extension actions map | ||
dynamicActionRegistry.registerExtensionAction(new ExtensionAction(action, extension.getId())); |
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.
If the action requires parameters then how will then be sent to the extension?
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.
The ExtensionAction
here is just a subclass of ActionType
which is an instance used as the key to a map. The value corresponding to this is an ExtensionTransportAction
with the same name, that references the ExtensionActionRequest
and ExtensionActionResponse
classes.
That ExtensionActionRequest
is how the details are communicated, with an action
string (a fully qualified class name that the extension will match to one of its existing getAction()
actions) and a byte array which will represent the parameters (serialized from the action's ActionRequest
). Those will be serialized and deserialized consistently.... currently with Writeable
but we'll probably switch to protobuf.
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.
+1 for protobufs
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.
Thanks for the refactor 👍
server/src/main/java/org/opensearch/client/node/NodeClient.java
Outdated
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
3829f15
to
7c8061e
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 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 #6734 +/- ##
============================================
- Coverage 70.74% 70.69% -0.05%
+ Complexity 59203 59195 -8
============================================
Files 4810 4809 -1
Lines 283487 283536 +49
Branches 40877 40885 +8
============================================
- Hits 200543 200440 -103
- Misses 66465 66624 +159
+ Partials 16479 16472 -7
... and 483 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
0c43ea3
to
c9c753c
Compare
Gradle Check (Jenkins) Run Completed with:
|
* Add dynamic action registry to ActionModule Signed-off-by: Daniel Widdis <[email protected]> * Update registration of transport actions Signed-off-by: Daniel Widdis <[email protected]> * Generate transport actions dynamically Signed-off-by: Daniel Widdis <[email protected]> * Refactor to combine registry internals Signed-off-by: Daniel Widdis <[email protected]> * Finally figured out the generics (or lack thereof) Signed-off-by: Daniel Widdis <[email protected]> * ExtensionProxyAction is dead! Long live ExtensionAction! Signed-off-by: Daniel Widdis <[email protected]> * Simplify ExtensionTransportActionHandler, fix compile issues Signed-off-by: Daniel Widdis <[email protected]> * Maybe tests will pass with this commit Signed-off-by: Daniel Widdis <[email protected]> * I guess you can't use null as a key in a map Signed-off-by: Daniel Widdis <[email protected]> * Lazy test setup, but this should finally work Signed-off-by: Daniel Widdis <[email protected]> * Add Tests Signed-off-by: Daniel Widdis <[email protected]> * Fix TransportActionRequestFromExtension inheritance Signed-off-by: Daniel Widdis <[email protected]> * Fix return type for transport actions from extensions Signed-off-by: Daniel Widdis <[email protected]> * Fix ParametersInWrongOrderError and add some preemptive null handling Signed-off-by: Daniel Widdis <[email protected]> * NPE is not expected result if params are in correct order Signed-off-by: Daniel Widdis <[email protected]> * Remove redundant class and string parsing, add success boolean Signed-off-by: Daniel Widdis <[email protected]> * Last fix of params out of order. Working test case! Signed-off-by: Daniel Widdis <[email protected]> * Code worked, tests didn't. This is finally done (I think) Signed-off-by: Daniel Widdis <[email protected]> * Add more detail to comments on immutable vs. dynamic maps Signed-off-by: Daniel Widdis <[email protected]> * Add StreamInput getter to ExtensionActionResponse Signed-off-by: Daniel Widdis <[email protected]> * Generalize dynamic action registration Signed-off-by: Daniel Widdis <[email protected]> * Comment and naming fixes Signed-off-by: Daniel Widdis <[email protected]> * Register method renaming Signed-off-by: Daniel Widdis <[email protected]> * Add generic type parameters Signed-off-by: Daniel Widdis <[email protected]> * Improve/simplify which parameter types get passed Signed-off-by: Daniel Widdis <[email protected]> * Revert removal of ProxyAction and changes to transport and requests Signed-off-by: Daniel Widdis <[email protected]> * Wrap ExtensionTransportResponse in a class denoting success Signed-off-by: Daniel Widdis <[email protected]> * Remove generic types as they are incompatible with Guice injection Signed-off-by: Daniel Widdis <[email protected]> * Fix response handling, it works (again) Signed-off-by: Daniel Widdis <[email protected]> * Fix up comments and remove debug logging Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> (cherry picked from commit 9febe10) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Add dynamic action registry to ActionModule * Update registration of transport actions * Generate transport actions dynamically * Refactor to combine registry internals * Finally figured out the generics (or lack thereof) * ExtensionProxyAction is dead! Long live ExtensionAction! * Simplify ExtensionTransportActionHandler, fix compile issues * Maybe tests will pass with this commit * I guess you can't use null as a key in a map * Lazy test setup, but this should finally work * Add Tests * Fix TransportActionRequestFromExtension inheritance * Fix return type for transport actions from extensions * Fix ParametersInWrongOrderError and add some preemptive null handling * NPE is not expected result if params are in correct order * Remove redundant class and string parsing, add success boolean * Last fix of params out of order. Working test case! * Code worked, tests didn't. This is finally done (I think) * Add more detail to comments on immutable vs. dynamic maps * Add StreamInput getter to ExtensionActionResponse * Generalize dynamic action registration * Comment and naming fixes * Register method renaming * Add generic type parameters * Improve/simplify which parameter types get passed * Revert removal of ProxyAction and changes to transport and requests * Wrap ExtensionTransportResponse in a class denoting success * Remove generic types as they are incompatible with Guice injection * Fix response handling, it works (again) * Fix up comments and remove debug logging --------- (cherry picked from commit 9febe10) Signed-off-by: Daniel Widdis <[email protected]> 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>
…oject#6734) * Add dynamic action registry to ActionModule Signed-off-by: Daniel Widdis <[email protected]> * Update registration of transport actions Signed-off-by: Daniel Widdis <[email protected]> * Generate transport actions dynamically Signed-off-by: Daniel Widdis <[email protected]> * Refactor to combine registry internals Signed-off-by: Daniel Widdis <[email protected]> * Finally figured out the generics (or lack thereof) Signed-off-by: Daniel Widdis <[email protected]> * ExtensionProxyAction is dead! Long live ExtensionAction! Signed-off-by: Daniel Widdis <[email protected]> * Simplify ExtensionTransportActionHandler, fix compile issues Signed-off-by: Daniel Widdis <[email protected]> * Maybe tests will pass with this commit Signed-off-by: Daniel Widdis <[email protected]> * I guess you can't use null as a key in a map Signed-off-by: Daniel Widdis <[email protected]> * Lazy test setup, but this should finally work Signed-off-by: Daniel Widdis <[email protected]> * Add Tests Signed-off-by: Daniel Widdis <[email protected]> * Fix TransportActionRequestFromExtension inheritance Signed-off-by: Daniel Widdis <[email protected]> * Fix return type for transport actions from extensions Signed-off-by: Daniel Widdis <[email protected]> * Fix ParametersInWrongOrderError and add some preemptive null handling Signed-off-by: Daniel Widdis <[email protected]> * NPE is not expected result if params are in correct order Signed-off-by: Daniel Widdis <[email protected]> * Remove redundant class and string parsing, add success boolean Signed-off-by: Daniel Widdis <[email protected]> * Last fix of params out of order. Working test case! Signed-off-by: Daniel Widdis <[email protected]> * Code worked, tests didn't. This is finally done (I think) Signed-off-by: Daniel Widdis <[email protected]> * Add more detail to comments on immutable vs. dynamic maps Signed-off-by: Daniel Widdis <[email protected]> * Add StreamInput getter to ExtensionActionResponse Signed-off-by: Daniel Widdis <[email protected]> * Generalize dynamic action registration Signed-off-by: Daniel Widdis <[email protected]> * Comment and naming fixes Signed-off-by: Daniel Widdis <[email protected]> * Register method renaming Signed-off-by: Daniel Widdis <[email protected]> * Add generic type parameters Signed-off-by: Daniel Widdis <[email protected]> * Improve/simplify which parameter types get passed Signed-off-by: Daniel Widdis <[email protected]> * Revert removal of ProxyAction and changes to transport and requests Signed-off-by: Daniel Widdis <[email protected]> * Wrap ExtensionTransportResponse in a class denoting success Signed-off-by: Daniel Widdis <[email protected]> * Remove generic types as they are incompatible with Guice injection Signed-off-by: Daniel Widdis <[email protected]> * Fix response handling, it works (again) Signed-off-by: Daniel Widdis <[email protected]> * Fix up comments and remove debug logging Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Valentin Mitrofanov <[email protected]>
Companion PR: SDK #588. This PR must be merged first.
Description
Adds a
DynamicActionRegistry
class and instance field to theActionModule
.TransportAction classes from OpenSearch and plugins are instantiated during Node bootstrap and injected as singletons.
Extensions have the need to proxy actions between extensions. For example, job scheduler needs to invoke actions on Anomaly Detector extension; Anomaly Detector may need to invoke actions on Alerting Extension; et. al. This capability is currently implemented with a single
ProxyAction
which relays these invocations blindly.This PR adds a registry for Extensions to register and unregister classes which extend
ActionType
andTransportAction
at times other than bootstrap.The benefit of using a subclass of
TransportAction
is that theclient.execute()
invocation processes these actions through ActionFilters. The first such filter processed is in the security plugin and this provides a single point of contact for future application of security features.Inspiration for some of these features: #4460 (but I did it my way)
Description of functionality:
ActionModule
creates an registry with two internal maps; one corresponds to theactions
map obtained from the injector, which is immutable, and an empty registry for addition (and potentially removal) of extension actionsgetActions()
extension point will be handled on OpenSearch by theExtensionTransportActionsHandler
which will register (in the registry) a custom subclass of theActionType
containing the fully qualified class name of the action on the Extensionclient.execute()
path. This path uses associated ActionFilters and the potential for future checks and security/identity integration.TransportAction
will send it to the Extension for further processing (TBD in Companion PR).Issues Resolved
Fixes SDK #107.
Companion PR implements SDK #525.
SDK #525 could be implemented using the existing
ProxyAction
setup that this PR replaces, but I believe the benefits of funneling all requests through a single touch point is a significant benefit and combined these efforts.Check List
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.