Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Avoid introducing empty ETag in response for keys created outside of S3 Gateway (e.g. via Ozone CLI).

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

How was this patch tested?

Added assertion in existing Robot test.

CI:
https://github.com/adoroszlai/ozone/actions/runs/8758764190

@adoroszlai adoroszlai self-assigned this Apr 20, 2024
Copy link
Contributor

@ivandika3 ivandika3 left a 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 fix. I left a comment.

this.replicationConfig = b.replicationConfig;
this.isFile = b.isFile;
this.eTag = b.eTag;
this.eTag = b.eTag != null && !b.eTag.isEmpty() ? b.eTag : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It might be better to use StringUtils#isNotEmpty

if (StringUtils.isNotEmpty(b.eTag) {
  this.eTag = b.eTag;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we might instead use an protobuf field existence check for eTag field on BasicOmKeyInfo#getFromProtobuf.

if (basicKeyInfo.hasETag()) {
  builder.setETag(basicKeyInfo.getETag());
}

Copy link
Contributor

@ivandika3 ivandika3 left a 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.

@adoroszlai adoroszlai merged commit 96fc70e into apache:master Apr 21, 2024
@adoroszlai
Copy link
Contributor Author

Thanks @ivandika3 for the review.

@adoroszlai adoroszlai deleted the HDDS-10719 branch April 21, 2024 06:33
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants