opensearch-sdk-java icon indicating copy to clipboard operation
opensearch-sdk-java copied to clipboard

[FEATURE] Refactor all sendFooRequest(TransportService transportService) calls into SDKTransportService

Open dbwiddis opened this issue 2 years ago • 1 comments

Is your feature request related to a problem?

While SDKClient handles REST calls to OpenSearch, the Proxy action implementation (see #525) requires transport action calls to OpenSearch, and thus require TransportService and OpenSearchNode objects.

The TransportService is initialized as part of the run() method after instantiation.

The OpenSearchNode isn't initialized until the ExtensionsRunner connects to OpenSearch.

Chicken, meet egg.

What solution would you like?

We need some sort of holder class that can be sent to the actions during ExtensionsRunner initialization and dependency injection. As part of work on #525 I've currently written an SDKTransportService class that works nicely for this purpose. With a setter, it can hold the (initialized) TransportService and OpenSearchNode (and UniqueId, too!) and greatly simplify these calls, particularly:

  • the only reason transportService is included as a parameter is for unit tests. That can be removed and tests can access the setter on the wrapper class to serve the same purpose.
  • there's no need to pass additional parameters like the OpenSearchNode or UniqueId that some of these calls need.
  • we could completely eliminate the getExtensionTransportService() getter, as well as getters for the OpenSearchNode and UniqueId, which could be moved to this class (they are rarely used outside these transport calls).

What alternatives have you considered?

I tried some other existing classes as the wrapper

  • SDKActionModule and ExtensionsRunner suffered from the same chicken and egg problem.
  • SDKClient worked, but I like the separation of purpose, SDKClient for REST, SDKTransportService for transport

Do you have any additional context?

Posting this request now, but it will touch a lot of classes (including tests) and would create big conflicts if overlapping in-progress work. It should wait for completion of:

  • #588 which will create this class
  • PR (TBD) implementing #17 which is working with initialization classes and would conflict with any new changes

dbwiddis avatar Mar 19 '23 03:03 dbwiddis

@dbwiddis I would like to take this issue

nassipkali avatar May 17 '23 16:05 nassipkali