Skip to content

Comments

HDDS-6709. Fix bucket usedBytes while versioning is true.#3388

Merged
captainzmc merged 1 commit intoapache:masterfrom
captainzmc:0506
May 10, 2022
Merged

HDDS-6709. Fix bucket usedBytes while versioning is true.#3388
captainzmc merged 1 commit intoapache:masterfrom
captainzmc:0506

Conversation

@captainzmc
Copy link
Member

@captainzmc captainzmc commented May 6, 2022

What changes were proposed in this pull request?

If Bucket versioning is set to true, the key should be able to keep multiple versions. And the keyToDelete in OMKeyCommitRequest will not be deleted. At this time the keyToDelete's usedBytes also should not detracted.

What is the link to the Apache JIRA

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

How was this patch tested?

UT has added.

@captainzmc
Copy link
Member Author

captainzmc commented May 6, 2022

Hi @kuenishi, keyToDelete was added in HDDS-5461. So can you help review this patch? cc @errose28

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nit on the test. @captainzmc up to you whether you want to merge this or #3286 first, so you can resolve possible merge conflicts in the one that gets merged second.

Copy link
Contributor

Choose a reason for hiding this comment

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

Test looks good, but can we use org.junit.jupiter.params.ParameterizedTest for the bucket layout types, instead of new test methods, similar to what the EC tests are doing for replication config in this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Let's change test using ParameterizedTest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Might be cleaner to combine the identical if conditions here and line 158

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Let's combine the identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well combine all the versioned bucker handling here and line 224

@kuenishi
Copy link
Contributor

kuenishi commented May 9, 2022

Good catch and thank you for fixing it! I'm afraid the same bug resides in committing multipart objects via S3 API: https://github.com/apache/ozone/blob/d1117ebe1689eb51695594c4a37a6f1a1672523f/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java#L225-L244

Fixing it in this issue or another one, either would be fine for me.

@captainzmc
Copy link
Member Author

Fixing it in this issue or another one, either would be fine for me.

Thanks @kuenishi for the tip, let's fix it in this PR.

Copy link
Contributor

@kuenishi kuenishi 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, all :)

@captainzmc
Copy link
Member Author

Thanks @errose28 @kerneltime @kuenishi for the review. Let's merge this.

@captainzmc captainzmc merged commit ff35102 into apache:master May 10, 2022
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.

4 participants