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] Refactor all sendFooRequest(TransportService transportService) calls into SDKTransportService #585

Open
dbwiddis opened this issue Mar 19, 2023 · 1 comment
Assignees
Labels
CCI Part of the College Contributor Initiative enhancement New feature or request good first issue Good for newcomers more-challenging Good issues for new contributors that present a small challenge

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Mar 19, 2023

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:

@dbwiddis dbwiddis added enhancement New feature or request untriaged labels Mar 19, 2023
@dbwiddis dbwiddis added the good first issue Good for newcomers label Mar 20, 2023
@dbwiddis dbwiddis added CCI Part of the College Contributor Initiative more-challenging Good issues for new contributors that present a small challenge labels Mar 24, 2023
@nassipkali
Copy link
Contributor

@dbwiddis I would like to take this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCI Part of the College Contributor Initiative enhancement New feature or request good first issue Good for newcomers more-challenging Good issues for new contributors that present a small challenge
Projects
None yet
Development

No branches or pull requests

3 participants