Hexiaoqiao commented on code in PR #4353: URL: https://github.com/apache/hadoop/pull/4353#discussion_r960686589
########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java: ########## @@ -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) { Review Comment: @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." ^_^ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org