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] Implement client.bulk in SDK Client #3192

Merged

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Oct 30, 2024

Description

Adds an SDKClient equivalent to client.bulk().

Summary of changes (helpful for reviewers):

  1. Added a new DataObjectRequest parent class. I originally started with a tagging interface just to help method signatures, but realized that four classes duplicated the same code for the index, id, and tenantId fields, so refactored those (and the associated builder code) into the superclass. Existing unit tests cover this change. caeea21
  2. Added a new DataObjectResponse parent class similar to above, refactoring the id and parser fields from the subclass. Existing unit tests cover this change. 445936b (part 1)
  3. Added new BulkDataObjectRequest and BulkDataObjectResponse classes and unit tests. Added a failed field to the response superclass to track failures/errors. 445936b (part 2)
  4. Added the interface and stubbed implementations for the SdkClient .bulk() method, and unit tests 22dccf4
  5. Implemented the new interface method in all 3 clients. The local and remote implementations use the OpenSearch bulk request natively. The DynamoDB implementation simply iterates the bulk request and calls existing code for the individual requests; this is not the most efficient but necessary to preserve ordering. If all the bulk requests are put and delete, it's possible to optimize this with DDB BulkWrite, which is a good future optimization; however since our primary use case uses update I'm deferring that complexity. ee30d21
  6. Additional refactoring in response to code review: move the details of ingest_took to a lower level 7f19078
  7. More parameters in the sdkclient request/response to handle the needed implementation 05a64d1
  8. Implementation and tests e2a5ec6

In addition there are several whitespace / import reordering changes due to spotless not having been run on the common module.

Will be handled integ tests in #2818

Check List

  • New functionality includes testing.
  • 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 had a problem deploying to ml-commons-cicd-env October 30, 2024 07:56 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env October 30, 2024 07:56 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env October 30, 2024 20:07 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env October 30, 2024 20:07 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env October 30, 2024 22:10 — with GitHub Actions Failure
@dhrubo-os dhrubo-os merged commit 3bbc700 into opensearch-project:feature/multi_tenancy Nov 21, 2024
7 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