-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-12824. Optimize container checksum read during datanode startup #8604
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
errose28
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.
Thanks for fixing this @aswinshakil. I have done a partial review, have not looked at the import/export changes yet.
...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java
Outdated
Show resolved
Hide resolved
...org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java
Outdated
Show resolved
Hide resolved
errose28
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.
Thanks for the updates @aswinshakil. I reviewed the import/export code as well this round, so there are some more comments in that area.
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Outdated
Show resolved
Hide resolved
...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java
Outdated
Show resolved
Hide resolved
...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/TarContainerPacker.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java
Outdated
Show resolved
Hide resolved
...t/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java
Outdated
Show resolved
Hide resolved
...est/java/org/apache/hadoop/ozone/dn/scanner/TestOnDemandContainerDataScannerIntegration.java
Outdated
Show resolved
Hide resolved
...service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java
Outdated
Show resolved
Hide resolved
...service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java
Outdated
Show resolved
Hide resolved
errose28
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.
Thanks for the fixes @aswinshakil. Looks like we should wait until #8565 is merged before proceeding with this one since they overlap in some areas. We should update TestContainerReader or somewhere similar for tests of loading containers when the tree and checksums may or may not exist.
...e/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
Show resolved
Hide resolved
errose28
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.
Left a few more comments mostly based on non-existent tree handling after #8565.
.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
Show resolved
Hide resolved
...r-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java
Show resolved
Hide resolved
...r-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java
Show resolved
Hide resolved
...service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java
Show resolved
Hide resolved
|
Test failure was not related to this change. It is being fixed in #8876 |
errose28
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.
Thanks for the updates. A few more comments, and this one still needs to be addressed.
...r-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
Outdated
Show resolved
Hide resolved
...-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java
Outdated
Show resolved
Hide resolved
|
@errose28 |
These tests are in |
errose28
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.
LGTM
* master: (55 commits) HDDS-13525. Rename configuration property to ozone.om.compaction.service.enabled (apache#8928) HDDS-13519. Reconciliation should continue if a peer datanode is unreachable (apache#8908) HDDS-13566. Fix incorrect authorizer class in ACL documentation (apache#8931) HDDS-13084. Trigger on-demand container scan when a container moves from open to unhealthy. (apache#8904) HDDS-13432. Accelerating Namespace Usage Calculation in Recon using - Materialised Approach (apache#8797) HDDS-13557. Bump jline to 3.30.5 (apache#8920) HDDS-13556. Bump assertj-core to 3.27.4 (apache#8919) HDDS-13543. [Docs] Design doc for OM bootstrapping process with snapshots. (apache#8900) HDDS-13541. Bump sonar-maven-plugin to 5.1.0.4751 (apache#8911) HDDS-13101. Remove duplicate information in datanode list output (apache#8523) HDDS-13528. Handle null paths when the NSSummary is initializing (apache#8901) HDDS-12990. (addendum) Generate tree from metadata when it does not exist during getContainerChecksumInfo call (apache#8881) HDDS-13086. Block duplicate reconciliation requests for the same container and datanode within the datanode. (apache#8905) HDDS-12990. Generate tree from metadata when it doesn't exist during getContainerChecksumInfo call (apache#8881) HDDS-12824. Optimize container checksum read during datanode startup (apache#8604) HDDS-13522. Rename axisLabel for No. of delete request received (apache#8879) HDDS-12196. Document ozone repair cli (apache#8849) HDDS-13514. Intermittent failure in TestNSSummaryMemoryLeak (apache#8889) HDDS-13423. Log reason for triggering on-demand container scan (apache#8854) HDDS-13466. Disable flaky TestOmSnapshotFsoWithNativeLibWithLinkedBuckets ...
What changes were proposed in this pull request?
In #8204 , during data node startup, we read the
<containerID>.treeto get the container checksum, which is used to update the in-memory container data. We want to store thisdataChecksumin RocksDB, because in a huge cluster, deserializing<containerID>.treefor every container on startup may have an impact. In this patchdataChecksumin RocksDB so that we don't have to deserialize the.treefile every time during DN restart..treefile during container import/export.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12824
How was this patch tested?
Updated existing tests