-
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-17194. Enhance the log message for striped block recovery #6094
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The failed unit test seems unrelated to the change. Hi Sirs @Hexiaoqiao @ayushtkn @ZanderXu Could you please help me review this minor changes when you have free time ? Thanks a lot~ |
@@ -427,26 +426,41 @@ protected void recover() throws IOException { | |||
// simply choose the one with larger length. | |||
// TODO: better usage of redundant replicas | |||
syncBlocks.put(blockId, new BlockRecord(id, proxyDN, info)); | |||
} else { | |||
if (LOG.isDebugEnabled()) { |
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.
No need to run isDebugEnabled() if you use {}
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.
Thanks @goiri help reivew and suggestions.
I will update later PR.
InterDatanodeProtocol.LOG.warn("Failed to recover block (block=" | ||
+ block + ", datanode=" + id + ")", e); | ||
InterDatanodeProtocol.LOG.warn("Failed to recover block (block={}, internalBlk={}, " + | ||
"datanode={})", block, internalBlk, id, e); | ||
} | ||
} | ||
checkLocations(syncBlocks.size()); | ||
|
||
final long safeLength = getSafeLength(syncBlocks); | ||
if (LOG.isDebugEnabled()) { |
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.
Avoid now that you use {}
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.
ok, I will update later it.
The PR has been updated. Hi @goiri please help review this changes again when you have free time. Thanks a lot~ |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
beb6ed6
to
4eedf9c
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The failed unit test seems unrelated to the change. Hi Sirs @goiri @Hexiaoqiao @ZanderXu please help me review this changes again when you have free time . Thanks a lot~ |
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.
@goiri I am bit curious on removing the isDebugEnabled checks.
Like here
- if (LOG.isDebugEnabled()) {
- LOG.debug("syncBlock replicaInfo: block=" + block +
- ", from datanode " + r.id + ", receivedState=" + rState.name() +
- ", receivedLength=" + r.rInfo.getNumBytes() +
- ", bestState=FINALIZED, finalizedLength=" + finalizedLength);
- }
+ LOG.debug("syncBlock replicaInfo: block={}, from datanode {}, receivedState={}, " +
+ "receivedLength={}, bestState=FINALIZED, finalizedLength={}",
+ block, r.id, rState.name(), r.rInfo.getNumBytes(), finalizedLength);
}
There are method calls like r.rInfo.getNumBytes()
these will be called & resolved irrespective whether debug logging is enabled or not, right? Even the concat on will be performed before passing to LOG.debug
, Wouldn't it be better to have a isDebug
gaurd where there are method calls or the log message is big & we use concat.
I am good with current as well
Thanks @ayushtkn for your review. So here we maybe need to update the code and add |
Description of PR
https://issues.apache.org/jira/browse/HDFS-17194
In order to convenient troubleshoot problems, consider add internalBlk information to the RecoveryTaskStriped#recover log message and optimize some log output