-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-13671. Namenode deletes large dir slowly caused by FoldedTreeSet#removeAndGet #3065
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
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
blockManager.getDatanodeManager().getDatanodes()) { | ||
for (long ucFileId : dataNode.getLeavingServiceStatus().getOpenFiles()) { | ||
// Sort open files | ||
LightWeightHashSet<Long> dnOpenFiles = dataNode.getLeavingServiceStatus().getOpenFiles(); |
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.
checkstyle
Failed tests pass locally. Now it is already for review! |
public DatanodeCommand blockReport(DatanodeRegistration registration, | ||
String poolId, StorageBlockReport[] reports, | ||
BlockReportContext context) | ||
String poolId, StorageBlockReport[] reports, BlockReportContext context) |
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.
NIT: unnecessary formatting change can be avoided.
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.
already fixed
💔 -1 overall
This message was automatically generated. |
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.
+1
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.
Now I've reviewed 35/38 files and the patch mostly looks good to me. I'll review the rest.
...op-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeStorageInfo.java
Outdated
Show resolved
Hide resolved
...op-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeStorageInfo.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockHasMultipleReplicasOnSameDN.java
Outdated
Show resolved
Hide resolved
...adoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Show resolved
Hide resolved
...oop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/ReplicaMap.java
Outdated
Show resolved
Hide resolved
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.
Reviewed all the files. Thank you!
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Show resolved
Hide resolved
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.
@AlphaGouGe Thank you!
💔 -1 overall
This message was automatically generated. |
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.
I added a few comments inline. But the change looks mostly good to me. Thanks for the work.
|
||
DatanodeDescriptor dn = storageInfo.getDatanodeDescriptor(); | ||
|
||
LOG.debug("Reported block {} on {} size {} replicaState = {}", block, dn, |
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.
This needs to be wrapped with if (LOG.isDebugEnabled()) as toString() of dn and block can be expensive.
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.
If the log level is not debug, toString() is not actually called.
http://www.slf4j.org/faq.html#logging_performance
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.
i have changed it back, removing if (LOG.isDebugEnabled())
// against the memory state. | ||
// See HDFS-6289 and HDFS-15422 for more context. | ||
queueReportedBlock(storageInfo, replica, reportedState, | ||
queueReportedBlock(storageInfo, block, reportedState, |
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.
block should be storedBlock?
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.
@xiaoyuyao you are right, it should be storedBlock
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.
I believe this is now correct, as it should be the reported block queue, rather than the stored block.
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.
@sodonnel Thanks for confirm
@xiaoyuyao Thanks for review, i have update this PR, take a look please. |
💔 -1 overall
This message was automatically generated. |
@xiaoyuyao Thanks for review! Is @AlphaGouGe 's fix OK? |
// against the memory state. | ||
// See HDFS-6289 and HDFS-15422 for more context. | ||
queueReportedBlock(storageInfo, replica, reportedState, | ||
queueReportedBlock(storageInfo, storedBlock, reportedState, |
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.
I this this change is incorrect - we should be queuing the details reported in the IBR, which I think is "replica" here. This change was made in HDFS-15422.
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.
That's right. This will undo the fix.
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.
sorry for missing the change on HDFS-15422, i have changed it back.
I think the patch looks good except for the things that were already pointed out by others. |
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, a few more comments added.
Thanks @kihwal @xiaoyuyao @sodonnel for review, i have updated the PR, take a look please. |
💔 -1 overall
This message was automatically generated. |
@AlphaGouGe Thanks for contribution. @aajisaka @sodonnel @kihwal @xiaoyuyao Thanks for careful review! All review comments are addressed, now it is ready to merge |
…#removeAndGet (apache#3065) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
…#removeAndGet (apache#3065) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/ReplicaMap.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockInfo.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
…oldedTreeSet#removeAndGet (apache#3065) (merge request !395) Squash merge branch 'THADOOP-187' into 'release-3.2.1-tq-0.2' * add UT testBlockListMoveToHead * HDFS-13671. Namenode deletes large dir slowly caused by FoldedTreeSet#removeAndGet (apache#3065)
https://issues.apache.org/jira/browse/HDFS-13671