-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Introduce Interfaces to Support atomic conditional-writes #18064
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
base: main
Are you sure you want to change the base?
Conversation
cccd8b3
to
72cb9c6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18064 +/- ##
============================================
- Coverage 72.50% 72.49% -0.01%
+ Complexity 67167 67113 -54
============================================
Files 5473 5473
Lines 310095 310097 +2
Branches 45062 45062
============================================
- Hits 224841 224813 -28
- Misses 66899 66915 +16
- Partials 18355 18369 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@x-INFiN1TY-x Please rebase your changes. |
Signed-off-by: Tanishq Ranjan <[email protected]>
72cb9c6
to
99e6943
Compare
Signed-off-by: Tanishq Ranjan <[email protected]>
eb43131
to
6461fe0
Compare
❌ Gradle check result for 6461fe0: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Flaky Test : : #17552
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add the required interfaces in AsyncMultiStreamBlobContainer?
❌ Gradle check result for 766204d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for a928b49: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Flaky Test : : #17937
|
Thanks for raising this, This PR is specifically focused on defining and aligning the necessary interfaces to support this capability across the relevant components. The implementation details—including multipart handling login via AsyncTransferManager—are addressed in the related PR : #18093. Please let me know if there’s anything further that needs to be adjusted, here's a diag detailing the intended downstream usage for the concerned interfaces. (Under review in #18093) |
private ConditionalWriteOptions(Builder builder) { | ||
this.ifNotExists = builder.ifNotExists; | ||
this.ifMatch = builder.ifMatch; | ||
this.ifUnmodifiedSince = builder.ifUnmodifiedSince; | ||
this.versionIdentifier = builder.versionIdentifier; | ||
this.unmodifiedSince = builder.unmodifiedSince; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there be case where the conditionalWriteOptions has multiple conditions and are contradicting? If yes do we need to have validation for same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the Concerned cloud storage providers support multiple conditional write options that can be employed concurrently, without needing to manually enforce checks for proper functioning, so we wouldn't require any validation for the same.
References :
-
AWS S3 conditional writes:
-
Azure Blob Storage conditional headers:
-
Google Cloud Storage generation preconditions:
Description
This PR introduces interfaces for conditional-write support and defines new option and response types, updating the
BlobContainer
interface, and extendingAsyncMultiStreamBlobContainer
to support asynchronous conditional uploads.New Types
ConditionalWriteOptions
— an object that explicitly models each supported precondition.ConditionalWriteResponse
— a typed result that carries provider-generated version identifiers and success status.Conditional APIs Interfaces for
BlobContainer
writeBlobConditionally
writeBlobWithMetadataConditionally
Async Conditional API
Introduces
asyncBlobUploadConditionally
toAsyncMultiStreamBlobContainer
.Provides the corresponding override in
AsyncMultiStreamEncryptedBlobContainer
to ensure encrypted uploads honor the same contract.Supported Preconditions
Atomic Write Workflow
Caller constructs a
ConditionalWriteOptions
specifying the desired precondition.On success, the blob (and optional metadata) are written, returning a
ConditionalWriteResponse
that includes the new version identifier for if-match writes.On failure, the provider’s standard precondition-failed exception is propagated.
Related Issues
Implements part of RFC #17763
Parent Meta Issue: [META] Implement Conditional APIs for Multi-Writer Detection
Supersedes Introduce Interface changes to Support atomic conditional-writes #18065
Check List
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.