Skip to content

Conversation

@ivandika3
Copy link
Contributor

@ivandika3 ivandika3 commented Feb 22, 2024

What changes were proposed in this pull request?

Currently copyObject does not seem to support eTag based on key's content. It uses a random UUID in CopyObjectResponse.

We need to change this to be the eTag based on the key content instead.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10403

How was this patch tested?

Unit tests, acceptance test, and manual tests.

Compatibilities

  • If the source key does not contain eTag field, the copied destination key will have an eTag field

Clean CI run: https://github.com/ivandika3/ozone/actions/runs/8001415597

@ivandika3 ivandika3 marked this pull request as ready for review February 22, 2024 09:31
@ivandika3
Copy link
Contributor Author

@kerneltime @myskov Could you help to review this when convenient? Thank you.

@myskov
Copy link
Contributor

myskov commented Feb 22, 2024

@vtutrinov could you also take a look, please?

@vtutrinov
Copy link
Contributor

@ivandika3 thanks for the patch! @myskov LGTM, +1

METRICS.updateCopyKeyMetadataStats(startNanos);
perf.appendMetaLatencyNanos(metadataLatencyNs);
writeLen = writeToStreamOutput(streamOutput, body, bufferSize, length);
eTag = DatatypeConverter.printHexBinary(body.getMessageDigest().digest())
Copy link
Contributor

Choose a reason for hiding this comment

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

As the eTag var is not used anywhere else, let's move eTag declaration here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Updated. PTAL.

@ivandika3 ivandika3 changed the title HDDS-10403. CopyObject should set eTag based on the key content HDDS-10403. CopyObject should set ETag based on the key content Feb 22, 2024
Copy link
Contributor

@myskov myskov left a comment

Choose a reason for hiding this comment

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

Thanks @ivandika3 for updating the patch. LGTM

@adoroszlai adoroszlai merged commit babf85c into apache:master Feb 23, 2024
@adoroszlai
Copy link
Contributor

Thanks @ivandika3 for the patch, @myskov, @vtutrinov for the review.

@ivandika3
Copy link
Contributor Author

Thank you for the reviews @myskov @vtutrinov and @adoroszlai for the merge.

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

Labels

s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants