-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HDFS-16593. Correct the BlocksRemoved metric on DataNode side #4353
Conversation
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao Could you help me review this patch? The failed UTs not caused by this modification, and has been solved in other jira. |
💔 -1 overall
This message was automatically generated. |
The failed UT |
💔 -1 overall
This message was automatically generated. |
💔 -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.
Sorry for the late response. Leave comment inline. FYI. Thanks.
@@ -2350,6 +2350,9 @@ private void invalidate(String bpid, Block[] invalidBlks, boolean async) | |||
removing = volumeMap.remove(bpid, invalidBlks[i]); | |||
addDeletingBlock(bpid, removing.getBlockId()); | |||
LOG.debug("Block file {} is to be deleted", removing.getBlockURI()); | |||
if (datanode.getMetrics() != null) { |
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.
Ah, I believe that this will make BlocksRemoved
more precisions. But I do not think it will be exact, consider that removing == null
which means that DataNode do not manage this pending delete replica, so we should not increase the metric. Maybe there are some other cases, not thought deeply. I just suggest we should consider if removing will be null.
Another point, line 2352 will throw NPE but not decide if removing will null or not. This is another issue, not related to this PR.
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.
@Hexiaoqiao Thanks for your review. I learned a lot and have updated this patch, please help me review it again.
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.
@ZanderXu Sorry to pollute this flow. After review this context carefully, I realize removing
will never be null actually. It will continue when meets null, and will never go to this logic (ref to Line2317). Another one, it is unnecessary to decide if datanode.getMetrics()
is null or not. The first version is without any issues.
I have made a common mistake "ask a programmer to review 10 lines of code he'll find 10 issues, ask him to do 500 lines and he'll say it looks good."(via twitter)
"Wish every PR could be merged without any issues reviewers feedback." ^_^
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.
@ZanderXu Copy, sir.
Another one, it is unnecessary to decide if datanode.getMetrics() is null or not
We need it, because hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
try to invalidate some blocks with null metrics. Or maybe we can modify the UT.
"Wish every PR could be merged without any issues reviewers feedback."
+1, and I will do my best to achieve it.
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.
We need it, because hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl try to invalidate some blocks with null metrics.
I found that there are many datanode.getMetrics()
calls without any null check in FsDatasetImpl, this is the same one. Not sure where will throw NPE, if that we should file another PR to fix them? Thanks.
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.
@Hexiaoqiao I have updated this code and fixed the failed UT org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
, please help me review it again. Thanks
💔 -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.
LGTM. +1 from my side. Let's wait what Yetus will say.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Committed to trunk. Thanks @ZanderXu . |
…#4353). Contributed by ZanderXu. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
Correct the BlocksRemoved metric on DataNode side.
When I read the code about
processCommandFromActive
, I guess that the BlocksRemoved metric is imprecise.So I think we should increase the metric after DN successfully deleted the block: