-
Notifications
You must be signed in to change notification settings - Fork 533
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(core): add if_not_exist
in OpWrite
#5305
Conversation
Signed-off-by: Keming <kemingy94@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kemingy for working on this.
core/src/types/operator/operator.rs
Outdated
@@ -1254,6 +1254,25 @@ impl Operator { | |||
/// # } | |||
/// ``` | |||
/// | |||
/// ## `if_not_exist` | |||
/// | |||
/// Set `if_not_exist` for this `write` request. This can be treated as a simplified version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, if_not_exist
is not the same feature as if_none_match
. For example, S3 only supports if_not_exist
but not if_none_match
. We should eliminate use cases like if_none_match("*")
to clarify this.
All existing tests and docs should replace by if_not_exist
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the current usage of if_none_match
. I can find it in op read/stat/write.
- Should we keep the tests since we still allow users to use this option?
- Do you mean that we should delete the
if_none_match
from the write-op-related docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should we keep the tests since we still allow users to use this option?
The tests using if_none_match("*")
should be removed since we can't guarantee that all services will support this.
- Do you mean that we should delete the
if_none_match
from the write-op-related docs?
We can retain this docs, but we need to remove the if_none_match("*")
example. Instead, we should use an etag
to clarify that we are accepting an etag
.
Signed-off-by: Keming <kemingy94@gmail.com>
Signed-off-by: Keming <kemingy94@gmail.com>
248630e
to
302ea26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, mostly looks good to me now!
Signed-off-by: Keming <kemingy94@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kemingy for working on this, really great!
Which issue does this PR close?
Closes #5286.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Add
op.write_with(path, bs).if_not_exists(true)