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/multi_tenancy] [WIP] SdkClient interface and implementation #2430

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented May 9, 2024

Description

Work In Progress

Creates an abstraction for index metadata, see opensearch-project/OpenSearch#13336 and initial implementation for Connector.

The near-term goal is proving out the flexibility of the interface.

Still TODO:

  • Update and Search APIs
  • Thorough exception handling and user error messages
  • More documentation

Issues Resolved

Part of #2358

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.

}

@Override
public CompletionStage<PutCustomResponse> putCustom(PutCustomRequest request) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about Update?

Copy link
Member Author

Choose a reason for hiding this comment

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

Coming soon to a PR near you, but prioritizing initial APIs to support multitenancy work.

new DeleteRequest(request.index(), request.id()),
ActionListener
.wrap(
r -> future.complete(new DeleteCustomResponse.Builder().id(r.getId()).deleted(r.getResult() == DELETED).build()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if the deletion fails? How are we sending the error response back to the customer?

Copy link
Member Author

Choose a reason for hiding this comment

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

The actionListener for the whole execution is being completed exceptionally. I have not yet gone through to more carefully identify exception possibilities and give more detailed user information. Again, this is an early-stage WIP.


/**
* Returns whether deletion was successful
* @return true if deletion was successful, false if the object was not found
Copy link
Collaborator

@dhrubo-os dhrubo-os May 9, 2024

Choose a reason for hiding this comment

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

what happens if the object was found but for some reason we couldn't delete.

Copy link
Member Author

Choose a reason for hiding this comment

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

The onFailure / exception path will be followed instead. This is the same behavior as the current DeleteResponse where you can have either DELETED (true) or NOT_FOUND (false) successful response.

@dhrubo-os
Copy link
Collaborator

I'm seeing:

/home/runner/work/ml-commons/ml-commons/common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java:43: error: HttpConnector is not abstract and does not override abstract method parse(XContentParser) in Custom
public class HttpConnector extends AbstractConnector {

Are you planning to look into this?

@dbwiddis
Copy link
Member Author

I'm seeing:

/home/runner/work/ml-commons/ml-commons/common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java:43: error: HttpConnector is not abstract and does not override abstract method parse(XContentParser) in Custom
public class HttpConnector extends AbstractConnector {

Are you planning to look into this?

Yeah, I missed actually implementing the parse() method in my own interface that needs to wrap the (static) xContent parsing methods in Connector. I'll push a fix tomorrow.

@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:26 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:38 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:38 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:38 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:39 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:39 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 04:39 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 15:19 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 10, 2024 15:19 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 07:03 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 07:03 — with GitHub Actions Error
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:03 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:03 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:03 — with GitHub Actions Inactive
@dbwiddis dbwiddis force-pushed the feature_multi_tenancy branch from da5f1f3 to 7d03c1e Compare May 17, 2024 07:45
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 07:45 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 08:34 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 08:34 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 08:34 — with GitHub Actions Error
@dbwiddis dbwiddis force-pushed the feature_multi_tenancy branch from 7d03c1e to fd22232 Compare May 17, 2024 16:13
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Failure
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Inactive
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env May 17, 2024 16:13 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 17:02 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 17:02 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env May 17, 2024 17:02 — with GitHub Actions Error
@peterzhuamazon peterzhuamazon deleted the branch opensearch-project:feature_multi_tenancy May 17, 2024 17:28
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.

4 participants