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] Create OpenSearchClient implementation in SDK Client and deprecate it #302

Closed
dbwiddis opened this issue Dec 29, 2022 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Dec 29, 2022

Is your feature request related to a problem?

To speed migration work for AD Plugin (and other existing plugins) we will create a client that can use the existing (NodeClient) API calls from those plugins.

What solution would you like?

Create a new getter in SDKClient that returns an instance Implementing OpenSearchClient that can be used by REST Handlers. Add deprecation annotations and javadocs clearly stating that it is provided for backwards compatibility and will eventually be removed.

What alternatives have you considered?

See #294

Do you have any additional context?

See opensearch-project/OpenSearch#5424 regarding eventual deprecation.

@dbwiddis dbwiddis added the enhancement New feature or request label Dec 29, 2022
@dbwiddis
Copy link
Member Author

dbwiddis commented Jan 3, 2023

Existing plugin implementations use this signature:

protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { ... }

Proposed implementation:

  1. Add another level of inheritance between BaseExtensionRestHandler and the individual implementations.
  2. Include an abstract prepareRequest() method with the above signature (but ExtensionRestRequest).
  3. Implement the necessary bits to translate from the routes() to handle the request/response (ExtensionRestResponse) handling similar to how is done in the handleRequest() method of BaseRestHandler.

@owaiskazi19
Copy link
Member

I created SDKNodeClient and created a dummy API in AD Extension to get it working but was facing issue with initialization of NodeClient.
Later, had a discussion with @saratvemulapalli regarding the NodeClient. Points discussed:

  1. Plugin uses NodeClient just to make a transport call to itself to retrieve the security credentials (eg. user)
  2. We don't need to make the above call because security will be part of REST layer anyway for extensions
  3. So instead of this, we will just return the REST response and the rest should be taken care by high level rest client
    This should unblock us for the POC of HLRC for AD extensions.

@dbwiddis
Copy link
Member Author

dbwiddis commented Jan 5, 2023

  1. So instead of this, we will just return the REST response and the rest should be taken care by high level rest client

I don't think that's such a simple replacement. client.execute() still has to do something to execute the transport; the arguments include 3 arguments: the action (a constant, we can deal with), the request (that the action needs), and an action listener for the response. (In this case it's created by a private method.)

So whatever client we create still has to pass the action identifier, and the request, to be executed by another class.

@dbwiddis
Copy link
Member Author

dbwiddis commented Jan 5, 2023

I believe what is necessary for step 3 above is to use a RemoteClusterAwareClient.

  • The AbstractClient superclass already implements execute() from the above plugin and delegates to the abstract method doExecute() which is the only implementation in the RemoteClusterAwareClient.
  • The RemoteClusterAwareClient is designed for exactly (and only) this purpose: to implement execute() via a TrasnportService.
  • The only change then needed is to change the signatures for prepareRequest() from a NodeClient to the RemoteClusterAwareClient.

@dbwiddis dbwiddis changed the title [FEATURE] Create NodeClient implementation in SDK Client and deprecate it [FEATURE] Create NodeClient or HLRC or RemoteClusterClient implementation in SDK Client and deprecate it Jan 5, 2023
@dbwiddis
Copy link
Member Author

dbwiddis commented Jan 5, 2023

Looking further into this:

  • RemoteClusterAwareClient looks like it could work if we had designed Extensions from scratch to run as a cluster. Since we didn't, it ends up being pretty heavyweight for this purpose.
  • The client implementation as a wrapper is a good example of future development we may want to do to wrap RestClient calls in a HLRC-syntax-compatible wrapper.
  • HLRC is still probably the way to go with some internal tweaks.

@dbwiddis dbwiddis changed the title [FEATURE] Create NodeClient or HLRC or RemoteClusterClient implementation in SDK Client and deprecate it [FEATURE] Create OpenSearchClient implementation in SDK Client and deprecate it Jan 6, 2023
@owaiskazi19
Copy link
Member

Raised a draft PR for @dbwiddis POC work for converting AD plugin to AD Extension using High Level Rest Client: opensearch-project/anomaly-detection#773

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