Skip to content

Comments

chore: enable conditional put by default for S3#7181

Merged
tustvold merged 3 commits intoapache:mainfrom
meteorgan:aws-default-conditional-put
Feb 27, 2025
Merged

chore: enable conditional put by default for S3#7181
tustvold merged 3 commits intoapache:mainfrom
meteorgan:aws-default-conditional-put

Conversation

@meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Feb 23, 2025

Which issue does this PR close?

Closes apache/arrow-rs-object-store#21

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

@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
@alamb alamb mentioned this pull request Mar 20, 2025
Closed
8 tasks
@kylebarron kylebarron mentioned this pull request Feb 26, 2025
Closed
@tustvold tustvold added the api-change Changes to the arrow API label Feb 26, 2025
@tustvold
Copy link
Contributor

tustvold 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?

@meteorgan
Copy link
Contributor Author

meteorgan commented Feb 26, 2025 •
edited
Loading

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

tustvold commented Feb 26, 2025

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

meteorgan commented Feb 26, 2025

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
@meteorgan meteorgan force-pushed the aws-default-conditional-put branch from 0048be2 to 0279ace Compare February 26, 2025 17:09
@kylebarron kylebarron mentioned this pull request Feb 26, 2025
Closed
@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
@alamb alamb mentioned this pull request Mar 14, 2025
Closed
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Mar 20, 2025
@meteorgan
chore: enable conditional put by default for S3 (apache#7181)
e43cc61 
* fix cargo clippy

* add Disabled variant to S3ConditionalPut

* restore object_store.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tustvold tustvold tustvold approved these changes

Assignees

No one assigned

Labels

api-change Changes to the arrow API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable conditional put by default for S3

2 participants

@meteorgan @tustvold