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.

Sign up for GitHub

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

Jump to bottom

feat: Add if_none_match for write #5129

Merged
merged 2 commits into from
Oct 8, 2024
Merged

feat: Add if_none_match for write #5129

merged 2 commits into from
Oct 8, 2024
+69 −0

Conversation

ForestLH
Copy link
Contributor

@ForestLH ForestLH commented Sep 19, 2024 •
edited
Loading

Which issue does this PR close?

Closes #5097

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Yes, add an optionif_none_match for write_with.

Xuanwo
Xuanwo reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mostly looks good to me. To make this feature work, we should also add a flag in Capability called write_with_if_none_match. Additionally, we can add a behavior test based on this.

core/src/types/operator/operator.rs Outdated Show resolved Hide resolved
core/src/types/operator/operator.rs Outdated Show resolved Hide resolved
core/src/types/operator/operator.rs
/// This prevents overwriting of existing objects with identical key names.
/// And must use the * (asterisk) value with this parameter
///
/// If file exists, an error with kind [`ErrorKind::ConditionNotMatch`] will be returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can also provide an example to demonstrate how to handle this error.

core/src/types/operator/operator.rs Outdated Show resolved Hide resolved
@ForestLH
Copy link
Contributor Author

It mostly looks good to me. To make this feature work, we should also add a flag in Capability called write_with_if_none_match. Additionally, we can add a behavior test based on this.

Thank you very much for your careful help. I have revised all your comments. 😄

@Xuanwo
Copy link
Member

Xuanwo commented Sep 23, 2024

Hi, the tests failed because the version we used for testing doesn't support this feature yet. The upstream added this feature in this commit.

Would you like to upgrade MinIO to the latest version, such as RELEASE.2024-09-13T20-26-02Z, in a new PR?

@Xuanwo Xuanwo changed the title feat:add if_none_match for write feat: Add if_none_match for write Sep 23, 2024
@ForestLH
Copy link
Contributor Author

Hi, the tests failed because the version we used for testing doesn't support this feature yet. The upstream added this feature in this commit.

Would you like to upgrade MinIO to the latest version, such as RELEASE.2024-09-13T20-26-02Z, in a new PR?

I'm sorry I didn't think of that. Thank you for your careful help.
And you mean upgrade MinIO version at here? Like this PR?

@tisonkun
Copy link
Member

tisonkun commented Oct 8, 2024

And you mean upgrade MinIO version at here? Like this #5142?

@ForestLH Yes. Please try it out.

FYI @Xuanwo reaction on comments won't send a notification so that the author may assume he/she isn't replied.

@ForestLH
Copy link
Contributor Author

ForestLH commented Oct 8, 2024

FYI @Xuanwo reaction on comments won't send a notification so that the author may assume he/she isn't replied.

Thanks for the tip!

@ForestLH
Copy link
Contributor Author

ForestLH commented Oct 8, 2024

Can you help me review the code, I have rebase the code to the latest.
Because I don't have a relevant environment, and I'm still learning OpenDAL, the code I write may not be comprehensive.
@Xuanwo

Xuanwo
Xuanwo approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR! Thanks you @ForestLH for the work, and also thanks @tisonkun for the review and notes.

@Xuanwo Xuanwo merged commit b92cfc4 into apache:main Oct 8, 2024
231 checks passed
@Xuanwo
Copy link
Member

Xuanwo commented Oct 8, 2024

if_none_match also supported by gcs and azblob, would you like to implement them too? cc @ForestLH

@ForestLH
Copy link
Contributor Author

ForestLH commented Oct 8, 2024

if_none_match also supported by gcs and azblob, would you like to implement them too? cc @ForestLH

Sure, I appreciate to be able to do this work! 😄 But I don't have gcs and azblob acounts. What better way than to sign up for gcs and azblob account?
ps, Shall I open another issue about this?

This was referenced Oct 21, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees
No one assigned
Labels
releases-note/feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new feature: Add docs about conditional write for s3
3 participants
@ForestLH @Xuanwo @tisonkun