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

Refactor SdkClient to a concrete class with delegate implementation #2638

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Jul 10, 2024

Description

Refactors SdkClient interface to a concrete class containing a delegate implementation. This allows it to be injected in the createComponents() extension point and available to @Inject constructors.

Uses Cluster Settings to determine which implementation is generated by the SdkClientFactory.

Adds SSL/TLS and Basic Auth to the remote client.

Adds an AWS OpenSearch Service client.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 00:21 — with GitHub Actions Error
@@ -86,162 +87,163 @@ public CompletionStage<SearchDataObjectResponse> searchDataObjectAsync(SearchDat
return CompletableFuture.completedFuture(searchResponse);
}
});
sdkClient = new SdkClient(sdkClientImpl);
Copy link
Collaborator

@dhrubo-os dhrubo-os Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can use sdkClient like this way for testing in algorithms module as we can't import SDKClientFactory class to algorithm module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but you'd still need the LocalClusterIndicesClient. One possibility is moving that default client to the common module since it doesn't need the Jackson stuff that the DDB Client does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be a easy workaround. Do you want to address that in this PR? Or in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably another PR. I may make another attempt to solve the jackson problems.

@@ -654,7 +649,7 @@ public Collection<Object> createComponents(
.getClusterSettings()
.addSettingsUpdateConsumer(MLCommonsSettings.ML_COMMONS_RAG_PIPELINE_FEATURE_ENABLED, it -> ragSearchPipelineEnabled = it);

LocalClusterIndicesClient localClusterIndicesClient = new LocalClusterIndicesClient(client, xContentRegistry);
SdkClient sdkClient = SdkClientFactory.createSdkClient(client, xContentRegistry, settings);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, so that means we don't need to do ?

ModulesBuilder modules = new ModulesBuilder();
modules.add(new SdkClientModule(client, xContentRegistry));

 injector = modules.createInjector();

 // Get the injected SdkClient instance from the injector
 SdkClient sdkClient = injector.getInstance(SdkClient.class);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. That created an injector that was different than the one OpenSearch processes with the list of objects from createComponents(). But the clients still had their runtime class so those would still have been bound to the subclasses, and this really just made a new/different SdkClient available to the various other classes in this method. We could have just instantiated an sdkClient without jumping through these hoops and gotten the same thing.

With the new code we have a concrete class SdkClient that is injected and available by its class key (which is what the separate module was doing) and hides the implementation as an internal field. Not the most object-oriented way to do things, but it works around the injection headaches.

@@ -0,0 +1,36 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these settings will eventually move to core, right?

Otherwise we usually keep our settings in one place:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are intended to be moved to wherever this whole package will eventually move (an OpenSearch module? Another plugin? The SDK for Java extensions repo? But it will not remain in ML Commons long term and those settings names will probably change when we relocate it.

@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 07:25 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 07:25 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 07:25 — with GitHub Actions Inactive
dbwiddis added 4 commits July 11, 2024 10:04
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis force-pushed the sdkclient-as-instance branch from 01cd95a to 00cd4ff Compare July 11, 2024 17:12
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env July 11, 2024 17:12 — with GitHub Actions Failure
@dhrubo-os dhrubo-os merged commit 7cd538e into opensearch-project:feature/multi_tenancy Jul 11, 2024
6 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants