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

[FEATURE] Replace TransportActions class functionality in SDKActionModule #467

Closed
dbwiddis opened this issue Feb 19, 2023 · 4 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Feb 19, 2023

Is your feature request related to a problem?

The TransportActions class contains an internal map that must be provided at instantiation. This map contains the same information in SDKActionModule.getActions() in a different format and is only relevant for an ActionExtension.

This requires unnecessary conditionals outside the class.

What solution would you like?

  1. Move the sendRegisterTransportActionsRequest method to the SDKActionModule
  2. Use the getActions() method of SDKActionModule to retrieve the necessary map
  3. Change the registration method to send only the necessary string info and not a full class
    4. Change the handler for actions sent from OpenSearch to use the SDKActionModule getActions() map (or use the injected transportAction(instance) lookup, or other approach) This handler doesn't exist. Opened [FEATURE] [DISCUSS] Implement Proxy Actions #525

What alternatives have you considered?

Creating the map outside the class, conditionally populating it, and then initializing it, which was done in #466

Adding a getter for the map to TransportActions so it can be initialized once externally (but this seems silly).

Do you have any additional context?

Consider whether we need this separate map signature and whether the values from SDKActionModule.getActions() are sufficient to serve the intended purpose.

@dbwiddis
Copy link
Member Author

dbwiddis commented Feb 19, 2023

@saratvemulapalli assigning this to you as you did #164, are continuing to iterate on dynamic actions, and may have better ideas than I did above for implementation. Feel free to reassign back to me if you prefer a direction and don't have time to do it -- just let me know which way to go.

@saratvemulapalli
Copy link
Member

@dbwiddis Trying to catchup from PR: #466

Looks like we do have SDKActionModule, which registers actions in the SDK.
Couple of questions (you can point me back to the PR, I just skimmed through):

  • How are these actions registered with ActionModule on OpenSearch(core)?
  • Having an ActionModule within the SDK will bring in state inconsistency problems, I am worried that the extension thinks it has registered the action but the namespace might have conflicted in OpenSearch.

@dbwiddis
Copy link
Member Author

@dbwiddis Trying to catchup from PR: #466

Looks like we do have SDKActionModule, which registers actions in the SDK. Couple of questions (you can point me back to the PR, I just skimmed through):

  • How are these actions registered with ActionModule on OpenSearch(core)?

The same way they already were (or weren't, due to bug #484) with your previous getActions() method.

Background:

You previously had a getActions() implementation on the SDK Extension interface that conflicted with the method signature for ActionPlugin on the SDK side.

In #434 I added the ActionExtension interface with the getActions() method signature from ActionPlugin:

default List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
    return Collections.emptyList();
}

We could not have an extension (e.g., AnomalyDetectionExtension) extending two interfaces (e.g., Extension and ActionExtension) with the same method name (e.g., getActions()) but having different return types, so as a temporary measure, I renamed your existing implementation to getActionMap():

- default Map<String, Class<? extends TransportAction<? extends ActionRequest, ? extends ActionResponse>>> getActions() {
+ default Map<String, Class<? extends TransportAction<? extends ActionRequest, ? extends ActionResponse>>> getActionsMap() {

Now enter #466, the intent was to keep the same action injection that existed in the OpenSearch side, so I copied over the appropriate parts of the ActionModule into the SDKActionModule. This enables the client.execute(action ... ) migration using a concrete "action" class tied to an injected instance of a "transport action" class, the same way current plugins use it. That SDKActionModule exposes the same getActions() method that the ActionModule exposes:

public Map<String, ActionHandler<?, ?>> getActions() {
    return actions;
}

This map has a 1-to-1 correspondence with your map (previous getActions(), now getActionsMap()):

- this.transportActions = new TransportActions(extension.getActionsMap());
+ Map<String, Class<? extends TransportAction<? extends ActionRequest, ? extends ActionResponse>>> transportActionsMap = new HashMap<>();
+     sdkActionModule.getActions()
+           .entrySet()
+           .stream()
+           .forEach(h -> { transportActionsMap.put(h.getKey(), h.getValue().getTransportAction()); });
+ this.transportActions = new TransportActions(transportActionsMap);

So this preserves exactly the same registration method you were already using, but since you never tried to register an extension action class, you didn't encounter the bug in #484. A class that exists only on the SDK side cannot be instantiated on the OpenSearch side.

So whatever registration we currently have/had is broken. But I know you're also working with Proxy Actions and there is some sort of registration envisioned there.

  • Having an ActionModule within the SDK will bring in state inconsistency problems, I am worried that the extension thinks it has registered the action but the namespace might have conflicted in OpenSearch.

This is no different conceptually with how we're handling Rest Handlers. We have a local registry of just the Extension's rest handlers, and we register them on OpenSearch's RestController with a single "forwarding" action. OpenSearch doesn't have a copy of the class, it just knows "this REST call goes to that extension". Same thing will be true with actions, we just need that unique (writeable) key registered and OpenSearch needs to know which extension owns that action.

@dbwiddis dbwiddis changed the title [FEATURE] Initialize action map inside TransportActions class and provide register api [FEATURE] Replace TransportActions class functionality in SDKActionModule Feb 28, 2023
@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 8, 2023

Completed in #524

@dbwiddis dbwiddis closed this as completed Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants