-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-10615. ETag change detected in S3A contract test #6519
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
Conversation
|
@ivandika3 @vtutrinov @whbing plese review |
ivandika3
left a comment
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 for catching and fixing this. Mostly LGTM.
Left a comment about wrapping double quotes for the response.
| keyMetadata.setETag("" + next.getModificationTime()); | ||
| String eTag = next.getMetadata().get(ETAG); | ||
| if (eTag != null) { | ||
| keyMetadata.setETag(eTag); |
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.
Shall we standardize so that the ETag returned needs to be surrounded by wrap in quotes? Similar to CopyObjectResponse?
From https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
However, it seems that the S3A pass even without the quotes. From what I briefly saw from AbstractContractEtagTest, the test does not assert the quotations of the ETag so it will pass regardless of the quotations.
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.
In the AWS page it is interesting that both Key and ETag are defined as string, but only that latter has quotes as part of the value.
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.
From what I understand the ETag is specified by RFC to always be surrounded by double quotes.
@vtutrinov Could you help confirm this?
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.
@ivandika3 sure thing, the RFC (https://datatracker.ietf.org/doc/html/rfc7232#section-2.3) says that the ETag header value is a quoted string
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.
Thanks for the pointers. Updated to wrap in quotes.
ivandika3
left a comment
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 for the update. LGTM +1.
|
Thanks @ivandika3, @vtutrinov for the review. |
(cherry picked from commit 92f2449)
(cherry picked from commit 92f2449)
What changes were proposed in this pull request?
Fix test failures related to in S3A contract test. The problem is that some parts of the code use "modification time" instead of content hash as ETag.
https://issues.apache.org/jira/browse/HDDS-10615
How was this patch tested?
ITestS3AContractEtagis enabled as part of the change.ITestS3AContractRenameis still failing with another error for FSO buckets only (HDDS-10665), but the ones related to ETag change are fixed. The test is passing with OBS bucket:https://github.com/adoroszlai/ozone/actions/runs/8661440172