Support native S3 conditional writes #6682

benesch · 2024-11-05T07:06:27Z

Add support for PutMode::Create and copy_if_not_exists on native AWS S3, which uses the underlying conditional write primitive that Amazon launched earlier this year 0.

The conditional write primitive is simpler than what's available in other S3-like products (e.g., R2), so new modes for s3_copy_if_not_exists and s3_conditional_put are added to select the native S3-specific behavior.

To maintain strict backwards compatibility (e.g. with older versions of LocalStack), the new behavior is not on by default. It must be explicitly requested by the end user.

The implementation for PutMode::Create is straightforward. The implementation of copy_if_not_exists is a bit more involved, as it requires managing a multipart upload that uses the UploadPartCopy operation, which was not previously supported by this crate's S3 client.

To ensure test coverage, the object store workflow now runs the AWS integration tests with conditional put both disabled and enabled.

Which issue does this PR close?

Fix #6285.

benesch · 2024-11-05T07:07:23Z

@tustvold are you the right person to review this? I saw you implemented quite a bit of the previous conditional put/get support.

benesch · 2024-11-05T07:15:17Z

To maintain strict backwards compatibility (e.g. with older versions of LocalStack), the new behavior is not on by default. It must be explicitly requested by the end user.

This isn't ideal, but it seemed best for the short term to avoid breaking backcompat for anyone who might be using a version of S3 that doesn't support CAS. In the long term, I think the ideal would definitely be to have the native S3 CAS support enabled by default (i.e. unless overridden explicitly by the user).

tustvold

Thank you, this looks good to me, mostly just some minor nits.

I agree with the decision to leave this off by default for a couple of reasons:

S3 compatible stores support these mechanisms with fewer drawbacks (e.g. full conditional update, native copy_if_not_exists, etc...) and users should prefer these
AWS may eventually bring S3 into feature parity with literally every other object store, at which point this will provide a coherent API to converge on

tustvold · 2024-11-05T15:45:55Z

object_store/src/integration.rs

@@ -651,6 +651,12 @@ pub async fn put_opts(storage: &dyn ObjectStore, supports_update: bool) {
    assert_eq!(b.as_ref(), b"a");

    if !supports_update {
+        let err = storage


tustvold · 2024-11-05T15:48:04Z

object_store/src/aws/precondition.rs

+    /// Encoded as `etag-create-only` ignoring whitespace.
+    ///
+    /// [announcement]: https://aws.amazon.com/about-aws/whats-new/2024/08/amazon-s3-conditional-writes/
+    ETagCreateOnly,


Suggested change

ETagCreateOnly,

ETagPutIfNotExists,

Maybe? I don't feel strongly though

Sure, works for me.

tustvold · 2024-11-05T15:51:21Z

object_store/src/aws/precondition.rs

+    /// WARNING: When using this mode, `copy_if_not_exists` does not copy
+    /// tags or attributes from the source object.


👍

I had a brief scan and couldn't see a way around this

tustvold · 2024-11-05T15:52:15Z

object_store/src/aws/client.rs

        }
    }
 }

+pub(crate) enum PutPartPayload<'a> {
+    Inline(PutPayload),


Suggested change

Inline(PutPayload),

Part(PutPayload),

Maybe?

Sure, no strong feels from me on the name here!

tustvold · 2024-11-05T15:53:31Z

object_store/src/aws/client.rs

+            PutPartPayload::Inline(payload) => request.with_payload(payload),
+            PutPartPayload::Copy(path) => request.header(
+                "x-amz-copy-source",
+                &format!("{}/{}", self.config.bucket, path),


Do we need to use encode_path here?

Ah yeah, seems like it, thanks!

object_store/src/aws/client.rs

tustvold · 2024-11-05T16:00:31Z

object_store/src/aws/mod.rs

+                    )
+                    .await
+                {
+                    Err(e @ Error::Precondition { .. }) => Err(Error::AlreadyExists {


Do we need to abort the multipart upload in this case? Does this happen automatically?

Oof, good call. It does not happen automatically. I added code to do a best effort multipart upload, and a docs note that this isn't guaranteed and that the user should configure automatic multipart expiration via a lifecycle rule.

tustvold · 2024-11-05T16:01:20Z

object_store/src/aws/precondition.rs

+    ///
+    /// WARNING: When using this mode, `copy_if_not_exists` does not copy
+    /// tags or attributes from the source object.
+    ///


Could possibly also link to the module docs around lifecycle rules for cleaning up incomplete multipart uploads - https://docs.rs/object_store/latest/object_store/aws/index.html#multipart-uploads

Good call, done!

calavera · 2024-11-05T19:09:54Z

object_store/src/aws/client.rs

-    CompleteMultipartRequest { source: crate::client::retry::Error },
+    CompleteMultipartRequest {
+        source: crate::client::retry::Error,
+        path: String,


should the path also be in the displayed error?

Good call, done.

Add support for `PutMode::Create` and `copy_if_not_exists` on native AWS S3, which uses the underlying conditional write primitive that Amazon launched earlier this year [0]. The conditional write primitive is simpler than what's available in other S3-like products (e.g., R2), so new modes for `s3_copy_if_not_exists` and `s3_conditional_put` are added to select the native S3-specific behavior. To maintain strict backwards compatibility (e.g. with older versions of LocalStack), the new behavior is not on by default. It must be explicitly requested by the end user. The implementation for `PutMode::Create` is straightforward. The implementation of `copy_if_not_exists` is a bit more involved, as it requires managing a multipart upload that uses the UploadPartCopy operation, which was not previously supported by this crate's S3 client. To ensure test coverage, the object store workflow now runs the AWS integration tests with conditional put both disabled and enabled. Fix apache#6285. [0]: https://aws.amazon.com/about-aws/whats-new/2024/08/amazon-s3-conditional-writes/

benesch

Thanks very much for the quick review, @tustvold! Addressed all your feedback, I believe.

benesch · 2024-11-05T20:44:54Z

object_store/src/aws/client.rs

-    CompleteMultipartRequest { source: crate::client::retry::Error },
+    CompleteMultipartRequest {
+        source: crate::client::retry::Error,
+        path: String,


Good call, done.

benesch · 2024-11-05T20:48:32Z

object_store/src/aws/client.rs

        }
    }
 }

+pub(crate) enum PutPartPayload<'a> {
+    Inline(PutPayload),


Sure, no strong feels from me on the name here!

benesch · 2024-11-05T21:16:49Z

object_store/src/aws/client.rs

+            PutPartPayload::Inline(payload) => request.with_payload(payload),
+            PutPartPayload::Copy(path) => request.header(
+                "x-amz-copy-source",
+                &format!("{}/{}", self.config.bucket, path),


Ah yeah, seems like it, thanks!

object_store/src/aws/client.rs

benesch · 2024-11-06T02:45:47Z

object_store/src/aws/precondition.rs

+    ///
+    /// WARNING: When using this mode, `copy_if_not_exists` does not copy
+    /// tags or attributes from the source object.
+    ///


Good call, done!

benesch · 2024-11-06T02:48:36Z

object_store/src/aws/precondition.rs

+    /// Encoded as `etag-create-only` ignoring whitespace.
+    ///
+    /// [announcement]: https://aws.amazon.com/about-aws/whats-new/2024/08/amazon-s3-conditional-writes/
+    ETagCreateOnly,


Sure, works for me.

benesch · 2024-11-06T03:01:26Z

object_store/src/aws/mod.rs

+                    )
+                    .await
+                {
+                    Err(e @ Error::Precondition { .. }) => Err(Error::AlreadyExists {


Oof, good call. It does not happen automatically. I added code to do a best effort multipart upload, and a docs note that this isn't guaranteed and that the user should configure automatic multipart expiration via a lifecycle rule.

To a version that supports conditional writes.

benesch · 2024-11-08T00:50:01Z

@tustvold if you can give me another re-run here I think we’ll be all green on CI. I fixed a minor clippy failure and upgraded to a version of localstack that supports conditional put.

benesch · 2024-11-08T14:55:09Z

Whew, all tests here are green! @tustvold let me know if there's anything else here you'd like to see, but from my perspective this is ready to (squash) merge.

tustvold

Just a small nit, then I think we're good to go. Thank you for this

tustvold · 2024-11-08T14:57:22Z

object_store/src/aws/mod.rs

+                    let part_id = self
+                        .client
+                        .put_part(to, &upload_id, 0, PutPartPayload::Copy(from))
+                        .await?;


I think this error should also trigger cleanup, might be worth encapsulating the steps into a function

It does, doesn't it? If anything in this async block (L306-329) returns an error, the cleanup on L336 should fire.

Oh I missed that this is wrapped in an async block, cunning

The fact that async blocks can function like try blocks is one of my favorite Rust tricks!

criccomini · 2024-11-08T20:00:23Z

Amazing. Any idea when this might go out 🔥

tustvold · 2024-11-08T20:27:06Z

#6596 tracks the next release

benesch · 2024-11-09T16:16:00Z

Thanks very much for the review and merge, @tustvold!

github-actions bot added the object-store Object Store Interface label Nov 5, 2024

This was referenced Nov 5, 2024

object_store: Conditional put and rename_if_not_exist on S3 #6285

Closed

Support S3 CAS now that PUT If-none-match: * is supported slatedb/slatedb#164

Open

tustvold approved these changes Nov 5, 2024

View reviewed changes

calavera reviewed Nov 5, 2024

View reviewed changes

benesch force-pushed the object-store-preconditions-native-s3 branch 2 times, most recently from dc2a634 to 075c524 Compare November 6, 2024 02:55

Address review feedback

9cf76bc

benesch force-pushed the object-store-preconditions-native-s3 branch from 075c524 to 9cf76bc Compare November 6, 2024 02:56

benesch commented Nov 6, 2024

View reviewed changes

kylebarron mentioned this pull request Nov 6, 2024

fsspec integration developmentseed/obstore#63

Merged

benesch added 2 commits November 7, 2024 17:35

Fix clippy failure

a974e76

Upgrade localstack in GitHub Actions

8bbf641

To a version that supports conditional writes.

tustvold reviewed Nov 8, 2024

View reviewed changes

tustvold merged commit c36ff79 into apache:master Nov 8, 2024
33 checks passed

kylebarron mentioned this pull request Nov 14, 2024

Support native S3 conditional writes developmentseed/obstore#89

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support native S3 conditional writes #6682

Support native S3 conditional writes #6682

benesch commented Nov 5, 2024

benesch commented Nov 5, 2024 •

edited

Loading

benesch commented Nov 5, 2024

tustvold left a comment

tustvold Nov 5, 2024

tustvold Nov 5, 2024

benesch Nov 6, 2024

tustvold Nov 5, 2024

tustvold Nov 5, 2024

benesch Nov 5, 2024

tustvold Nov 5, 2024

benesch Nov 5, 2024

tustvold Nov 5, 2024

benesch Nov 6, 2024

tustvold Nov 5, 2024

benesch Nov 6, 2024

calavera Nov 5, 2024

benesch Nov 5, 2024

benesch left a comment

benesch Nov 5, 2024

benesch Nov 5, 2024

benesch Nov 5, 2024

benesch Nov 6, 2024

benesch Nov 6, 2024

benesch Nov 6, 2024

benesch commented Nov 8, 2024

benesch commented Nov 8, 2024

tustvold left a comment

tustvold Nov 8, 2024

benesch Nov 8, 2024

tustvold Nov 8, 2024

benesch Nov 9, 2024

criccomini commented Nov 8, 2024

tustvold commented Nov 8, 2024

benesch commented Nov 9, 2024

		/// WARNING: When using this mode, `copy_if_not_exists` does not copy
		/// tags or attributes from the source object.

Support native S3 conditional writes #6682

Support native S3 conditional writes #6682

Conversation

benesch commented Nov 5, 2024

Which issue does this PR close?

benesch commented Nov 5, 2024 • edited Loading

benesch commented Nov 5, 2024

tustvold left a 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

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

benesch left a 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

benesch commented Nov 8, 2024

benesch commented Nov 8, 2024

tustvold left a 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

criccomini commented Nov 8, 2024

tustvold commented Nov 8, 2024

benesch commented Nov 9, 2024

benesch commented Nov 5, 2024 •

edited

Loading