HDFS-17556. Avoid adding block to neededReconstruction repeatedly in decommission #6896
+90
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@lfxy Thanks for your report and PR. It looks good to me. Please check the failed unit test and if related to this PR. It's better to add another test to cover this case if possible. Thanks again. |
|
@Hexiaoqiao Thanks for your review! The failed unit test is org.apache.hadoop.hdfs.server.datanode.TestDirectoryScanner |
|
💔 -1 overall
This message was automatically generated. |
lfxy
force-pushed
the
feature/HDFS-17556
branch
from
5526269 to
2bd848d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@Hexiaoqiao I have add new UT to cover this case, and please review again. Thanks! |
|
Well work, Leave some comments inline, Thanks~ |
haiyang1987
reviewed
Aug 25, 2024
...adoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java
Show resolved
Hide resolved
|
@haiyang1987 Thank you for your suggestion. I have update the UT. |
|
🎊 +1 overall
This message was automatically generated. |
lfxy
force-pushed
the
feature/HDFS-17556
branch
from
01c9c84 to
d9edab0
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
I think you may mean that add block to pendingReconstruction in two place is duplicated.
I think in (2), when we add timed out block to pendingReconstruction, we will use isNeededReconstruction to check. Will it really be repeated? |
|
@zhengchenyu I think it's wrong you described about pendingReconstruction. |
|
@lfxy |
|
@zhengchenyu We should check (1) -> (2), but not (4) -> (5), because (4)->(5) is not only used in decommission flow. |
|
@Hexiaoqiao @haiyang1987 Hi, Could this commit be merged? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HDFS-17556
In decommission and maintenance process, before added to BlockManager::neededReconstruction block will be check if it has been added. The check contains if block is in BlockManager::neededReconstruction or in PendingReconstructionBlocks::pendingReconstructions as below code.
But it also need to check if it is in PendingReconstructionBlocks::timedOutItems. Or else DatanodeAdminDefaultMonitor will add block to BlockManager::neededReconstruction repeatedly if block time out in PendingReconstructionBlocks::pendingReconstructions.
if (!blockManager.neededReconstruction.contains(block) && blockManager.pendingReconstruction.getNumReplicas(block) == 0 && blockManager.isPopulatingReplQueues()) { // Process these blocks only when active NN is out of safe mode. blockManager.neededReconstruction.add(block, liveReplicas, num.readOnlyReplicas(), num.outOfServiceReplicas(), blockManager.getExpectedRedundancyNum(block)); }