object-store: support real S3's If-Match semantics#6801
object-store: support real S3's If-Match semantics#6801benesch wants to merge 1 commit intoapache:mainfrom
Conversation
As of today [0] S3 now supports the If-Match for in-place conditional
writes. This commit adjusts the existing support for
S3ConditionalPut::Etag mode for compatibility with real S3's particular
semantics, which vary slightly from MinIO and R2. Specifically:
* Real S3 can occasionally return 409 Conflict when concurrent
If-Match requests are in progress. These requests need to be
retried.
* Real S3 returns 404 Not Found instead of 412 Precondition Failed
when issuing an If-Match request against an object that does not
exist.
Fix #6799.
[0]: https://aws.amazon.com/about-aws/whats-new/2024/11/amazon-s3-functionality-conditional-writes/
benesch
force-pushed
the
object-store-s3-put-if-match
branch
from
f2ba56a to
5535851
Compare
benesch
changed the title
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
Makes sense to me, thank you.
| { | ||
| // Real S3 reports NotFound rather than PreconditionFailed when the | ||
| // object doesn't exist. Convert to PreconditionFailed for | ||
| // consistency with R2. This also matches what the HTTP spec |
You bet! Anything else you need from me to merge this? |
|
No, I think this looks good, but given the localstack update may land shortly I was going to hold this until then just to ensure there aren't any gremlins lurking |
Makes sense! In that case would you prefer if I closed this PR in favor of #6802, so there's just one PR to review and merge after the new localstack release comes out? Or is it still useful to have both PRs? |
|
Let's keep it for now just in case there's some unforeseen hickup with the localstack release, but yes presuming localstack lands we'll probably just merge the other PR |
|
Sounds good. |
|
Since all seems to be working with the latest version of localstack, I'm going to go ahead and close this in favor of #6802. Can always reopen this if necessary! |
2 participants
As of today (0) S3 now supports the If-Match for in-place conditional writes. This commit adjusts the existing support for S3ConditionalPut::Etag mode for compatibility with real S3's particular semantics, which vary slightly from MinIO and R2. Specifically:
Real S3 can occasionally return 409 Conflict when concurrent If-Match requests are in progress. These requests need to be retried.
Real S3 returns 404 Not Found instead of 412 Precondition Failed when issuing an If-Match request against an object that does not exist.
I tested this against real S3, since localstack doesn't yet support
If-Match. #6802 is a follow-up PR that will update CI to test this new support once we have a version of localstack to test against.Fix apache/arrow-rs-object-store#33.
Which issue does this PR close?
apache/arrow-rs-object-store#33
Are there any user-facing changes?
None beyond what's described in the PR description.
cc @tustvold