[Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations #2520

dbwiddis · 2024-06-07T23:31:33Z

Description

Adds update method to SdkClient interface (full document update, does not do scripts)
Implements update method on Local and Remote clients
Migrates Connector Update action to use SdkClient for the updates

NOTE: this PR builds on commit 22a0ce5 which represents PR #2507 . Reviewers may want to focus attention on commits 100d71a and e90b553

Will rebase this PR against the branch when #2507 is merged.

Issues Resolved

Continuation of PR #2459

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.

dhrubo-os · 2024-06-14T15:14:57Z

common/src/main/java/org/opensearch/sdk/SdkClient.java

+            Throwable cause = e.getCause();
+            if (cause instanceof InterruptedException) {
+                Thread.currentThread().interrupt();
+            }
+            // Rethrow unchecked Exceptions
+            if (cause instanceof RuntimeException) {
+                throw (RuntimeException) cause;
+            } else {
+                throw new OpenSearchException(cause);                
+            }


if all these are same for every methods, may be we can put that in a method to reuse?

ExceptionsHelper.convertToRuntime() would be perfect for this but takes an Exception as an argument... guess I have to copy it. 🤷

arjunkumargiri · 2024-06-14T15:45:55Z

common/src/main/java/org/opensearch/sdk/UpdateDataObjectResponse.java

+    private final ShardId shardId;
+    private final ShardInfo shardInfo;


Why is this information required? Can this be null for non-OpenSearch data stores?

No, it can not be null, you have asked this same question before and it was answered here: #2459 (comment)

arjunkumargiri · 2024-06-14T15:46:53Z

common/src/main/java/org/opensearch/sdk/SdkClient.java

+                Thread.currentThread().interrupt();
+            }
+            // Rethrow unchecked Exceptions
+            if (cause instanceof RuntimeException) {


Given that CompletionException extends RuntimeException wouldn't this condition always be true?

Cause is already unwrapped from CompletionException here.

arjunkumargiri · 2024-06-14T15:48:28Z

plugin/src/main/java/org/opensearch/ml/action/connector/GetConnectorTransportAction.java

-                            log.error("Failed to get ML connector {}", connectorId, cause);
-                            actionListener.onFailure(new RuntimeException(cause));
+                            log.error("Failed to get ML connector " + connectorId, cause);
+                            if (cause instanceof Exception) {


Same as above, wouldn't all exceptions be instance of Exception

Cause is of type Throwable so yes this should probably almost always be true, unless someone decides to throw an Error sometime in the future but this check is needed for safe type casting.

Signed-off-by: Daniel Widdis <[email protected]>

dhrubo-os · 2024-06-14T23:05:05Z

plugin/src/main/java/org/opensearch/ml/sdkclient/LocalClusterIndicesClient.java

-                throw notFound;
-            } catch (Exception e) {
-                throw new OpenSearchException(e);
+            } catch (IOException e) {


so for other unchecked exceptions, it will automatically throw the exception what we have in SdkClient?

@dhrubo-os Correct, any other exception will be a non-null Throwable supplied to the completable future. It's tested in LocalClusterIndicesClientTests.testGetDataObject_Exception() passing an UnsupportedOperationException that would bypass this catch block.

…onnector Implementations (opensearch-project#2520) * Restore original exception handling expectations Signed-off-by: Daniel Widdis <[email protected]> * Add UpdateDataObject to interface and implementations Signed-off-by: Daniel Widdis <[email protected]> * Implement UpdateConnector action Signed-off-by: Daniel Widdis <[email protected]> * Move CompletionException handling to a common method Signed-off-by: Daniel Widdis <[email protected]> * Add tests for SDKClient exceptions refactored from Transport Action Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]>

* AWS DDB SDK client support for remote data store Signed-off-by: Arjun kumar Giri <[email protected]> * AWS DDB SDK client support for remote data store Signed-off-by: Arjun kumar Giri <[email protected]> * multi-tenancy for models (create, get, delete, update) + update connector (#2546) * multi-tenancy for models (create, get, delete) Signed-off-by: Dhrubo Saha <[email protected]> * added update connector + update model Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * [Feature/multi_tenancy] Add source map to GetDataObjectResponse (#2489) * Add source map to GetDataObjectResponse Signed-off-by: Daniel Widdis <[email protected]> * Add test for map getter in clients Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * [Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations (#2520) * Restore original exception handling expectations Signed-off-by: Daniel Widdis <[email protected]> * Add UpdateDataObject to interface and implementations Signed-off-by: Daniel Widdis <[email protected]> * Implement UpdateConnector action Signed-off-by: Daniel Widdis <[email protected]> * Move CompletionException handling to a common method Signed-off-by: Daniel Widdis <[email protected]> * Add tests for SDKClient exceptions refactored from Transport Action Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * Addressed CR comment Signed-off-by: Arjun kumar Giri <[email protected]> * Added javadoc based on feedback Signed-off-by: Arjun kumar Giri <[email protected]> --------- Signed-off-by: Arjun kumar Giri <[email protected]> Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: arjunkumargiri <[email protected]> Co-authored-by: Dhrubo Saha <[email protected]> Co-authored-by: Daniel Widdis <[email protected]>

* AWS DDB SDK client support for remote data store Signed-off-by: Arjun kumar Giri <[email protected]> * AWS DDB SDK client support for remote data store Signed-off-by: Arjun kumar Giri <[email protected]> * multi-tenancy for models (create, get, delete, update) + update connector (#2546) * multi-tenancy for models (create, get, delete) Signed-off-by: Dhrubo Saha <[email protected]> * added update connector + update model Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * [Feature/multi_tenancy] Add source map to GetDataObjectResponse (#2489) * Add source map to GetDataObjectResponse Signed-off-by: Daniel Widdis <[email protected]> * Add test for map getter in clients Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * [Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations (#2520) * Restore original exception handling expectations Signed-off-by: Daniel Widdis <[email protected]> * Add UpdateDataObject to interface and implementations Signed-off-by: Daniel Widdis <[email protected]> * Implement UpdateConnector action Signed-off-by: Daniel Widdis <[email protected]> * Move CompletionException handling to a common method Signed-off-by: Daniel Widdis <[email protected]> * Add tests for SDKClient exceptions refactored from Transport Action Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]> * Addressed CR comment Signed-off-by: Arjun kumar Giri <[email protected]> * Added javadoc based on feedback Signed-off-by: Arjun kumar Giri <[email protected]> * Set tenant ID for predict request Signed-off-by: Arjun kumar Giri <[email protected]> * Simplify instantiating Data Object Request/Response builders (#2608) Signed-off-by: Daniel Widdis <[email protected]> * Addressed comments Signed-off-by: Arjun kumar Giri <[email protected]> --------- Signed-off-by: Arjun kumar Giri <[email protected]> Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: arjunkumargiri <[email protected]> Co-authored-by: Dhrubo Saha <[email protected]> Co-authored-by: Daniel Widdis <[email protected]>

dbwiddis requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and sam-herman as code owners June 7, 2024 23:31

dbwiddis temporarily deployed to ml-commons-cicd-env June 7, 2024 23:31 — with GitHub Actions Inactive

dbwiddis had a problem deploying to ml-commons-cicd-env June 7, 2024 23:31 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env June 7, 2024 23:31 — with GitHub Actions Failure

dbwiddis mentioned this pull request Jun 13, 2024

AWS DDB SDK client support for remote data store #2553

Merged

4 tasks

dhrubo-os reviewed Jun 14, 2024

View reviewed changes

dbwiddis temporarily deployed to ml-commons-cicd-env June 14, 2024 16:19 — with GitHub Actions Inactive

arjunkumargiri reviewed Jun 14, 2024

View reviewed changes

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 17:11 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 17:11 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 17:11 — with GitHub Actions Error

Restore original exception handling expectations

1507a6f

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 19:34 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 19:34 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 19:34 — with GitHub Actions Error

dbwiddis added 2 commits June 14, 2024 13:36

Implement UpdateConnector action

f26971f

Signed-off-by: Daniel Widdis <[email protected]>

Move CompletionException handling to a common method

1acfd82

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the update-api branch from f02adc8 to 1acfd82 Compare June 14, 2024 20:36

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 20:36 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 20:36 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 20:36 — with GitHub Actions Failure

Add tests for SDKClient exceptions refactored from Transport Action

f45cda5

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis temporarily deployed to ml-commons-cicd-env June 14, 2024 22:17 — with GitHub Actions Inactive

dbwiddis temporarily deployed to ml-commons-cicd-env June 14, 2024 22:18 — with GitHub Actions Inactive

dhrubo-os mentioned this pull request Jun 14, 2024

[Feature/multi_tenancy] Restore original exception handling expectations #2507

Closed

5 tasks

dhrubo-os reviewed Jun 14, 2024

View reviewed changes

dhrubo-os approved these changes Jun 14, 2024

View reviewed changes

dhrubo-os merged commit 4d0fbdc into opensearch-project:feature/multi_tenancy Jun 14, 2024
9 checks passed

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 23:10 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env June 14, 2024 23:10 — with GitHub Actions Error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations #2520

[Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations #2520

dbwiddis commented Jun 7, 2024

dhrubo-os Jun 14, 2024

dbwiddis Jun 14, 2024

arjunkumargiri Jun 14, 2024

dbwiddis Jun 14, 2024

arjunkumargiri Jun 14, 2024

dbwiddis Jun 14, 2024

arjunkumargiri Jun 14, 2024

dbwiddis Jun 14, 2024

dhrubo-os Jun 14, 2024

dbwiddis Jun 14, 2024

		private final ShardId shardId;
		private final ShardInfo shardInfo;

[Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations #2520

[Feature/multi_tenancy] Add UpdateDataObject interface, Client, and Connector Implementations #2520

Conversation

dbwiddis commented Jun 7, 2024

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment