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

chore: enable conditional put by default for S3 #7181

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

meteorgan
Copy link
Contributor

Which issue does this PR close?

Closes #7080

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

set S3ConditionalPut::ETagMatch as the default value for conditional_put in AmazonS3Builder

@github-actions github-actions bot added the object-store Object Store Interface label Feb 23, 2025
@meteorgan meteorgan marked this pull request as draft February 23, 2025 13:44
@meteorgan meteorgan force-pushed the aws-default-conditional-put branch 2 times, most recently from 1200e25 to 116c4ff Compare February 23, 2025 14:08
@meteorgan meteorgan marked this pull request as ready for review February 23, 2025 14:40
@tustvold tustvold added the api-change Changes to the arrow API label Feb 26, 2025
@tustvold
Copy link
Contributor

One potential issue with the approach in this PR is I don't think there is a way now to turn off conditional put support, perhaps we need to add a Disabled variant?

@meteorgan
Copy link
Contributor Author

meteorgan commented Feb 26, 2025

One potential issue with the approach in this PR is I don't think there is a way now to turn off conditional put support, perhaps we need to add a Disabled variant?

Why and when would we need to disable conditional put ? and if it's disabled, what actions can we take ? Returning NotImplemented doesn't help.

@tustvold
Copy link
Contributor

return NotImplemented doesn't help.

The scenario is an S3-compatible that doesn't support the headers, and simply ignores them, I could see people wanting to disable support so that they get an error instead of silently incorrect behaviour.

@meteorgan
Copy link
Contributor Author

The scenario is an S3-compatible that doesn't support the headers, and simply ignores them, I could see people wanting to disable support so that they get an error instead of silently incorrect behaviour.

Ok, Let me add a Disabled variant to S3ConditionalPut.

@meteorgan meteorgan force-pushed the aws-default-conditional-put branch from 116c4ff to d1e2c76 Compare February 26, 2025 14:19
@tustvold tustvold merged commit fc360a9 into apache:main Feb 27, 2025
14 checks passed
@meteorgan meteorgan deleted the aws-default-conditional-put branch February 27, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable conditional put by default for S3
2 participants