[FEATURE] Remove dependent code associated with the Client thread pool/context in AD for creating detector

[owaiskazi19]

Is your feature request related to a problem?

As we are just focusing on one user for creating a detector, the dependency of client can be removed from AD.

What solution would you like?

A clear and concise description of what you want to happen.

What alternatives have you considered?

A clear and concise description of any alternative solutions or features you've considered.

Do you have any additional context?

Add any other context or screenshots about the feature request here.

[dbwiddis]

Based on some lessons learned in #23 I'm going to be re-evaluating the scope of this issue when I start it next week. Essentially all dependencies on the ThreadContext need to be removed. User is done in #23 and this issue addresses Client. There is also a dependency on Settings.

Settings may be included in Client and if so this issue should be sufficient. If not I'll create a new issue.

[dbwiddis]

Removing the Client interface will remove extensive functionality from the existing plugin. Methods such as index(), get(), search(), execute(), and admin() will no longer be available. Is it the intention to completely remove these from the extension without a replacement?

[dbwiddis]

I've tried a couple of experiments, and all are very invasive and require commenting out large portions of code in multiple classes, essentially making many methods no-ops or removing them entirely (and then removing all the tests dependent on them, etc.). But I think there's a simpler solution which effectively accomplishes the same thing.

The problem starts here:

/**
 * Entry point of AD plugin.
 */
public class AnomalyDetectorPlugin extends Plugin

By extending Plugin we inherit and have to override the createComponents() method which is how a Client object is initially populated; all other uses of Client depend on the object passed in to this method.

Removing the override inherits a method that creates an empty list; that essentially means there are no components (including the client) which is probably overkill.

Alternately we could keep the signature as is, but pass our own client (SdkClient?) that we can override the index, get, search, execute, and admin methods as appropriate to use the SDK.

[dbwiddis]

The only actual use of a specific Client implementation is in the parameter passed to the createComponents() method of this (or any) Plugin implementation. As outlined in this comment it is tightly integrated into the plugin's functionality as the methods defined in the OpenSearchClient interface (execute()) and Client interface (search(), index(), get(), and admin()).

Given that we will be implementing extensions via REST API, we should extend an existing Client which already implements that functionality. I initially started looking at the clients included in OpenSearch core such as the RestHighLevelClient and RestClient as well as the Java Client, but despite the similar naming, these are not subclasses of Client or OpenSearchClient nor do they generate/create or otherwise access the needed interfaces.

Looking at the Client javadoc linked above, if we are to extend an existing client it would have to be one of AbstractClient, FilterClient, NodeClient, OriginSettingClient, ParentTaskAssigningClient, or RestCancellableNodeClient.

Extending AbstractClient seems the most obvious for now, as it provides basic implementations for all the functionality. I'm not ruling out any of the others (which are subclasses) and we can consider adding another layer of subclassing in the future if we want/need their functionality. So we would:

• Create an SdkClient (or ExtensionClient) in this repository, extending AbstractClient.
  • We will pass this client to the createComponents() method of what is currently the AD Plugin but will become the AD Extension, launched via ExtensionsRunner for any given extension.
  • This inheritance will give us admin() and execute(), as well as search, index() and get() which delegate to execute(), already implemented.
    • One hard part is in writing the Response and Request to be passed to these methods, but the existing plugin code already does most of this.
    • The other hard part is making sure we have appropriate ResponseHandlers in the SDK.

SO it does not look like we would need to add much to the existing Client. But, we do want to remove some functionality, namely, the ThreadContext! There are several instances of:

try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
  // ... stuff possibly using context ...
}

Some of them used the thread context for User related information. Those dependencies have been removed. There are a few where the context object is passed along, such as AnomalyDetectorJobTransportAction where the context is eventually passed to startDetector() but the only use of it is to restore() and undo the initial stash. Perhaps at some point there was a User pulled from the context.

TLDR:

• We should consider a new issue to outline the additional work required in Request and Response needed for the Client interface's execute() method.
• We should create a new issue to:
  • Write code create an SdkClient extending AbstractClient in this repo.
    • The constructor requires settings and a ThreadPool; we should write a single-arg constructor which passes null as the ThreadPool for the superconstructor.
      • consider overriding threadPool() to throw an exception instead of returning null.
  • Write code to instantiate this client and pass it to the plugin (now) / extension (later) createComponent() method.
• I will do work on this issue to remove all calls to client.threadPool() so this plugin/extension is no longer dependent on that method.

[dbwiddis]

Completed in opensearch-project/anomaly-detection#619