Details
-
Type:
Improvement
-
Status: Resolved
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 2.8.0, 3.0.0-alpha1
-
Component/s: datanode
-
Labels:None
-
Target Version/s:
-
Hadoop Flags:Reviewed
Issue Links
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| -1 | pre-patch | 18m 13s | Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 1 new or modified test files. |
| +1 | javac | 7m 54s | There were no new javac warning messages. |
| +1 | javadoc | 10m 33s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 23s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 25s | The applied patch generated 8 new checkstyle issues (total was 512, now 493). |
| -1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. |
| +1 | install | 1m 29s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 34s | The patch built with eclipse:eclipse. |
| -1 | findbugs | 2m 34s | The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings. |
| +1 | native | 3m 13s | Pre-build of native portion |
| -1 | hdfs tests | 50m 53s | Tests failed in hadoop-hdfs. |
| 97m 15s |
| Reason | Tests |
|---|---|
| FindBugs | module:hadoop-hdfs |
| Failed unit tests | hadoop.hdfs.TestReplaceDatanodeOnFailure |
This message was automatically generated.
Thanks Walter Su. Nice work!
I've few comments, please see
- In unit tests probably you could use GenericTestUtils.assertExceptionContains for asserting the exception messages rather than swallow it.
+ try { + spyTask.recover(); + } catch(IOException e){ + // IOException: All datanodes failed + }
- class RecoveryTask does not need to be public. Also, how about renaming the class to RecoveryTaskContiguous?
- Do you have any plans to incorporate the comments discussed in
HDFS-9173here in this jira? - Please take care the checkstyle warnings related to the patch.
Adding one more comment,
- Please add @InterfaceAudience.Private to the newly added BlockRecoveryWorker class.
Thanks Rakesh R! Uploaded 04 patch address all your comments.
Many checkstyle issues already exist before. I'll fix them when I move the code.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| -1 | pre-patch | 19m 48s | Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 1 new or modified test files. |
| +1 | javac | 9m 4s | There were no new javac warning messages. |
| +1 | javadoc | 11m 58s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 26s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 36s | The applied patch generated 3 new checkstyle issues (total was 513, now 489). |
| -1 | whitespace | 0m 1s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. |
| +1 | install | 1m 38s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 38s | The patch built with eclipse:eclipse. |
| -1 | findbugs | 2m 57s | The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings. |
| +1 | native | 3m 40s | Pre-build of native portion |
| -1 | hdfs tests | 70m 56s | Tests failed in hadoop-hdfs. |
| 122m 46s |
| Reason | Tests |
|---|---|
| FindBugs | module:hadoop-hdfs |
| Failed unit tests | hadoop.hdfs.server.datanode.fsdataset.impl.TestInterDatanodeProtocol |
| hadoop.hdfs.TestRecoverStripedFile | |
| hadoop.hdfs.server.namenode.TestFSImageWithSnapshot |
This message was automatically generated.
Thanks Walter for the work. The 04 patch looks good overall. Some minors:
- Datanode#recoverBlocks doesn't need a return value. We can also consider replacing the method with a getter for blockRecoveryWorker and getBlockRecoveryWorker().recoverBlocks.
- The structure of BlockRecoveryWorker can also be simplified. Following the example of ErasureCodingWorker we can make RecoveryTaskContiguous itself a daemon. But since this is just a refactor JIRA, we can address this issue and the previous one in a separate JIRA.
- Since we are explicitly naming RecoveryTaskContiguous, should also throw unsupported exception for striped blocks?
+ for(RecoveringBlock b : blocks) { + try { + logRecoverBlock(who, b); + RecoveryTaskContiguous task = new RecoveryTaskContiguous(b); + task.recover();
- I guess changes in PBHelper, DatanodeManager, and FSNamesystem are all misc optimizations not directly related to this refactor? PBHelper and DatanodeManager changes LGTM. But I haven't fully reviewed FSNamesystem. It looks a decent size optimization by itself. How about do it separately?
The changes in commitBlockSynchronization is to utilize DatanodeManager.getDatanodeStorageInfos(..) which knows DatanodeID.EMPTY_DATANODE_ID. But in fact EMPTY_DATANODE_ID only used for striped file.
I meant to separte all non-related changes of HDFS-9173 to here. Sorry for the confusing, I still hope to include the changes in here.
Thanks for the clarification Walter. IIUC DatanodeManager#getDatanode(DatanodeID nodeID) returns null for EMPTY_DATANODE_ID, and that should be trimmed in trimmedTargets and trimmedStorages right? In your updated logic, nothing will be done to null entries in trimmedStorages anyway. Do we still need the change?
Some nits if we decide to do the above change:
- The current logic is not trimming null entries from newtargets so trimmedStorages needs a better name
- Why do we deep copy storedBlock but not truncatedBlock?
if(copyTruncate) { - storageInfo.addBlock(truncatedBlock, truncatedBlock); + trimmedStorages[i].addBlock(truncatedBlock, truncatedBlock); } else { - storageInfo.addBlock(storedBlock, storedBlock); + Block bi = new Block(storedBlock); + trimmedStorages[i].addBlock(storedBlock, bi); } - We can take the chance to change newtargets to newTargets, and change the for (int i = 0; i < trimmedStorages.length loop to foreach style.
The old way trims the invalid targets, shrinks the array ( by using a tmp ArrayList).
In the patch, I want to replace the invalid targets, or EMPTY targets with null, but don't shrinks the array. (So trimmedStorages is not a proper name here?)
Short story, they are for striped block.
Long story, UC.replicas are sorted in the order of block index, so as the newTargets sent by Client/DN. So we don't have to extend updatePipeline(..) or commitBlockSynchronization(..) protocol to send indices[]. A missing block is null in the newTargets slot, in this way we know its index.
A little awkward here...hmm...Thanks for deeply looking. Maybe it's inappropriate to keep the changes here, and I should keep the changes in HDFS-9173, how do you think?
1.Datanode#recoverBlocks doesn't need a return value. We can also consider replacing the method with a getter for blockRecoveryWorker and getBlockRecoveryWorker().recoverBlocks.
1. Done. 2. Removed FSNamesystem changes. Uploaded 05 patch.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| -1 | pre-patch | 18m 17s | Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 1 new or modified test files. |
| +1 | javac | 7m 51s | There were no new javac warning messages. |
| +1 | javadoc | 10m 23s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 27s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 28s | The applied patch generated 1 new checkstyle issues (total was 286, now 264). |
| -1 | whitespace | 0m 1s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. |
| +1 | install | 1m 33s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 34s | The patch built with eclipse:eclipse. |
| -1 | findbugs | 2m 33s | The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings. |
| +1 | native | 3m 12s | Pre-build of native portion |
| -1 | hdfs tests | 51m 35s | Tests failed in hadoop-hdfs. |
| 97m 58s |
| Reason | Tests |
|---|---|
| FindBugs | module:hadoop-hdfs |
| Failed unit tests | hadoop.hdfs.server.datanode.TestDataNodeHotSwapVolumes |
This message was automatically generated.
Thanks for working on this, Walter! The patch looks pretty good to me. One question is: since DataNode#blockRecoveryWorker is not declared as final, can we make sure the BPServiceActor thread can always see its non-null value when calling getBlockRecoveryWorker?
One question is: since DataNode#blockRecoveryWorker is not declared as final, can we make sure the BPServiceActor thread can always see its non-null value when calling getBlockRecoveryWorker?
No. uploaded 06 patch to address that.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| -1 | pre-patch | 18m 30s | Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 1 new or modified test files. |
| +1 | javac | 8m 27s | There were no new javac warning messages. |
| +1 | javadoc | 11m 34s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 28s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 32s | The applied patch generated 1 new checkstyle issues (total was 286, now 264). |
| -1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. |
| +1 | install | 1m 36s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 37s | The patch built with eclipse:eclipse. |
| -1 | findbugs | 2m 52s | The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings. |
| +1 | native | 3m 31s | Pre-build of native portion |
| +1 | hdfs tests | 53m 26s | Tests passed in hadoop-hdfs. |
| 102m 37s |
| Reason | Tests |
|---|---|
| FindBugs | module:hadoop-hdfs |
This message was automatically generated.
Thanks Walter for the update. +1 on the latest patch (listing a few nits that can be done as follow-on). Jing Zhao Do you have further comments?
- BlockRecoveryWorker#recoverBlocks doesn't need a return value
- Even though DN now initializes the recovery worker before block pool manager, BPOfferService can still use a null check at dn.getBlockRecoveryWorker()
+1 on the latest patch. Thanks Walter!
Walter Su Could you set a target version? There's a conflict when backporting to branch-2
SUCCESS: Integrated in Hadoop-trunk-Commit #8719 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8719/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #595 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/595/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #608 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/608/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
SUCCESS: Integrated in Hadoop-Yarn-trunk #1331 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1331/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
SUCCESS: Integrated in Hadoop-Hdfs-trunk #2484 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2484/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #546 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/546/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
FAILURE: Integrated in Hadoop-Mapreduce-trunk #2538 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2538/)
HDFS-9255. Consolidate block recovery related implementation into a (zhz: rev e287e7d14b838a866ba03d895fa35819999d7c09)
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockRecoveryWorker.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
This patch introduced a Findbugs warning, which is tracked in HDFS-9401.
This message was automatically generated.