HDDS-13921. Conditional Copy (CopyObject)#10207
HDDS-13921. Conditional Copy (CopyObject)#10207YutaLin wants to merge 8 commits intoapache:masterfrom
Conversation
|
Hi @ivandika3, @peterxcli |
peterxcli
left a comment
There was a problem hiding this comment.
LGTM. Could you also add s3 sdk v1/v2 test?
|
Hi @peterxcli, thanks for the review! |
peterxcli
left a comment
There was a problem hiding this comment.
@YutaLin I think this patch only handle the read side condition check, but we also need dest side:
Destination Object Conditions (AWS Release 2.37.0):
If-None-Match: "*" - Copy only if destination does NOT exist Prevents unintended overwrites of destination object
Error Codes: 412 Precondition Failed - When any condition is not met
Hi @peterxcli, the conditional read side check is Let me know if I miss anything, thanks! |
| * Context for evaluating preconditions, defining which headers to check | ||
| * and how to handle cache validation scenarios. | ||
| */ | ||
| enum PreconditionContext { |
There was a problem hiding this comment.
how about also add write in here?
There was a problem hiding this comment.
We don't need it now because IF-Match/IF-None-Match have same behavior on GET/HEAD/CopyObject.
There was a problem hiding this comment.
Could you please elaborate? which of their behaviour is the same? Thanks!
There was a problem hiding this comment.
All write operations use the same headers and have the same behavior:
- If-None-Match: * → create only if key doesn't exist
- If-Match: → overwrite only if ETag matches
- Both throw 412 on failure
There was a problem hiding this comment.
oh, I got your point, write header are always the same, this class is only for distinguishing the copy/read request header(they have same suffix)
However the PreconditionContext name is too generic, let's rename it to sth like ReadCondition.
thanks for explanation, then I think only thing remain is the test coverage for them. I saw the copy request builder has following we should use the cover the write part in sdk v2: public final String ifMatch() {
return this.ifMatch;
}
public final String ifNoneMatch() {
return this.ifNoneMatch;
}v1 also need |
Hi @peterxcli CI: https://github.com/YutaLin/ozone/actions/runs/25591732427 |
|
Hi @peterxcli, thanks for the feedback |
What changes were proposed in this pull request?
support CopyObject conditions include
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13921
How was this patch tested?
add conditional copy object test and run action ci (https://github.com/YutaLin/ozone/actions/runs/25467526822)