HDDS-7258. Cleanup the allocated but uncommitted blocks #3778

Xushaohong · 2022-09-26T04:05:32Z

What changes were proposed in this pull request?

We found that the blocks the client committed may not be equal to the allocated(when EC stripe fails), and the uncommitted blocks would remain forever as orphan blocks as no key maps to them anymore.

Here I use a tricky solution that considers these blocks as repeated/ overwritten key blocks, and thus they would be deleted in the existing deletion path.

https://docs.google.com/document/d/1Oi1zPKdmvA7DFwIcUWz6zw_p-pY3D1L76gyV37jQT94/edit#

The prototype of the design.

What is the link to the Apache JIRA

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

How was this patch tested?

UT and Manual test in the test environment.

sodonnel · 2022-09-26T17:04:20Z

Could you please describe how these blocks are being detected in the PR description - like a small design overview?

Xushaohong · 2022-09-27T08:52:37Z

Could you please describe how these blocks are being detected in the PR description - like a small design overview?

Sure, will update later, I am working on fixing some issues found by UT.

Xushaohong · 2022-09-27T12:14:12Z

https://docs.google.com/document/d/1Oi1zPKdmvA7DFwIcUWz6zw_p-pY3D1L76gyV37jQT94/edit#

The prototype of the design.

kaijchen · 2022-10-08T06:38:28Z

Thanks for the work @Xushaohong.
Is it possible to delete those blocks directly instead of relying on OpenKeyCleanupService?
I'm worrying about too much coupling here.

Xushaohong · 2022-10-08T06:50:10Z

Thx for the question~ @kaijchen
The case for OpenKeyCleanupService is not related to the logic here. They should be two cases as I mentioned in the DOC.

kaijchen · 2022-10-08T06:54:12Z

Thx for the question~ @kaijchen
The case for OpenKeyCleanupService is not related to the logic here. They should be two cases as I mentioned in the DOC.

Sorry I misread your purposal. The pseudoKeyInfo is moved into DeleteKeyTable
instead of staying at OpenKeyTable, which sounds reasonable to me.

kaijchen

Thanks for the work @Xushaohong, mostly LGTM.
I have a few improvements inlined.

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java

Xushaohong · 2022-10-18T11:27:15Z

Hi, all! I have done the refactor of this PR. Could you take a look ~

errose28

Thanks for the fix @Xushaohong this is a good improvement to have.

errose28 · 2022-10-18T20:35:04Z

...one-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java

It looks like the purpose of this new getKeyLocation(int, int) method is to create block lists with some overlap. However the client should never be able to commit blocks that were not already allocated in the open key. See #2108, which modified OMKeyInfo#verifyAndGetKeyLocations to drop the extra blocks and added a corresponding unit test in this class. Therefore in this test I think we only need to test the client committing a subset of the blocks it allocated. We could use the original getKeyLocation(int) method to make the allocated list, copy this, and just remove a block or two to create the commit list without needing a new method.

Thx @errose28 for the review!
Actually committing the unknown block from the client side is still possible, #2108 this work is on the server side.
But I have done some improvements like what you said, PTAL~

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java

kaijchen · 2022-10-20T12:43:32Z

Thanks @Xushaohong for updating the patch. Filtering uncommitted blocks in verifyAndGetKeyLocations is a good idea. For clearity, it's better to return a Pair<updatedBlockLocations, uncommittedBlocks> instead, or create a inner class.

errose28

Thanks for the updates @Xushaohong. Looks good overall just some minor comments left.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

kaijchen

LGTM, thanks @Xushaohong for updating this PR.

errose28

Thanks for the improvement @Xushaohong lgtm

Xushaohong · 2022-10-26T01:58:31Z

@kaijchen @errose28 Thx for the review! :)

* master: (718 commits) HDDS-7342. Move encryption-related code from MultipartCryptoKeyInputStream to OzoneCryptoInputStream (apache#3852) HDDS-7413. Fix logging while marking container state unhealthy (apache#3887) Revert "HDDS-7253. Fix exception when '/' in key name (apache#3774)" HDDS-7396. Force close non-RATIS containers in ReplicationManager (apache#3877) HDDS-7121. Support namespace summaries (du, dist & counts) for legacy FS buckets (apache#3746) HDDS-7258. Cleanup the allocated but uncommitted blocks (apache#3778) HDDS-7381. Cleanup of VolumeManagerImpl (apache#3873) HDDS-7253. Fix exception when '/' in key name (apache#3774) HDDS-7182. Add property to control RocksDB max open files (apache#3843) HDDS-7284. JVM crash for rocksdb for read/write after close (apache#3801) HDDS-7368. [Multi-Tenant] Add Volume Existence check in preExecute for OMTenantCreateRequest (apache#3869) HDDS-7403. README Security Improvement (apache#3879) HDDS-7199. Implement new mix workload Read/Write Freon command (apache#3872) HDDS-7248. Recon: Expand the container status page to show all unhealthy container states (apache#3837) HDDS-7141. Recon: Improve Disk Usage Page (apache#3789) HDDS-7369. Fix wrong order of command arguments in Nonrolling-Upgrade.md (apache#3866) HDDS-6210. EC: Add EC metrics (apache#3851) HDDS-7355. non-primordial scm fail to get signed cert from primordial SCM when converting an unsecure cluster to secure (apache#3859) HDDS-7356. Update SCM-HA.zh.md to match the English version (apache#3861) HDDS-6930. SCM,OM,RECON should not print ERROR and exit with code 1 on successful shutdown (apache#3848) ... Conflicts: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestLegacyReplicationManager.java

kerneltime requested review from errose28, kerneltime and sodonnel September 26, 2022 16:04

umamaheswararao requested a review from adoroszlai September 28, 2022 16:31

Xushaohong marked this pull request as draft September 29, 2022 08:49

Xushaohong force-pushed the HDDS-7258 branch from 7f55a28 to fbab4e8 Compare October 8, 2022 06:17

Xushaohong marked this pull request as ready for review October 8, 2022 06:19

kaijchen reviewed Oct 8, 2022

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java Outdated Show resolved Hide resolved

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java Outdated Show resolved Hide resolved

Xushaohong force-pushed the HDDS-7258 branch from 81acffe to 15fd96c Compare October 17, 2022 08:42

errose28 reviewed Oct 19, 2022

View reviewed changes

Xushaohong added 2 commits October 20, 2022 15:49

HDDS-7258. Cleanup the allocated but uncommitted blocks

8610ea0

Use set instead of List

a371952

Xushaohong force-pushed the HDDS-7258 branch from ce41792 to a45cdd3 Compare October 20, 2022 07:50

Improve the efficiency

c8c03bc

Xushaohong force-pushed the HDDS-7258 branch from a45cdd3 to c8c03bc Compare October 20, 2022 09:12

fix ut comment

e194c6e

kaijchen reviewed Oct 20, 2022

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java Outdated Show resolved Hide resolved

errose28 reviewed Oct 21, 2022

View reviewed changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java Outdated Show resolved Hide resolved

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java Show resolved Hide resolved

fix comment V2

7468e20

Xushaohong force-pushed the HDDS-7258 branch from a0a3578 to 7468e20 Compare October 21, 2022 03:57

Xushaohong added 2 commits October 24, 2022 11:09

Make pseudoKeyInfo set dataSize todo

a094dfe

fix ut

e7d3806

fix ut 2.0

873acb5

Xushaohong requested review from errose28 and removed request for adoroszlai, kerneltime and sodonnel October 24, 2022 10:33

errose28 reviewed Oct 24, 2022

View reviewed changes

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java Outdated Show resolved Hide resolved

fix spell error

d139f04

kaijchen approved these changes Oct 25, 2022

View reviewed changes

errose28 approved these changes Oct 25, 2022

View reviewed changes

errose28 merged commit dfc13a0 into apache:master Oct 25, 2022

kaijchen pushed a commit to kaijchen/ozone that referenced this pull request Oct 26, 2022

HDDS-7258. Cleanup the allocated but uncommitted blocks (apache#3778)

b2ff74f

HDDS-7258. Cleanup the allocated but uncommitted blocks #3778

HDDS-7258. Cleanup the allocated but uncommitted blocks #3778

Uh oh!

Conversation

Xushaohong commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

sodonnel commented Sep 26, 2022

Uh oh!

Xushaohong commented Sep 27, 2022

Uh oh!

Xushaohong commented Sep 27, 2022

Uh oh!

kaijchen commented Oct 8, 2022

Uh oh!

Xushaohong commented Oct 8, 2022

Uh oh!

kaijchen commented Oct 8, 2022

Uh oh!

kaijchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Xushaohong commented Oct 18, 2022

Uh oh!

errose28 left a comment

Choose a reason for hiding this comment

Uh oh!

errose28 Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

Xushaohong Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaijchen commented Oct 20, 2022

Uh oh!

errose28 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaijchen left a comment

Choose a reason for hiding this comment

Uh oh!

errose28 left a comment

Choose a reason for hiding this comment

Uh oh!

Xushaohong commented Oct 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xushaohong commented Sep 26, 2022 •

edited

Loading