-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-25570 On largish cluster, "CleanerChore: Could not delete dir..… #2949
Conversation
+ "happening, use following exception when asking on mailing list.", | ||
type, dir, ioe); | ||
if (LOG.isTraceEnabled()) { | ||
LOG.info("Could not delete {} under {}; will retry. If it keeps happening, " + |
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.
should this be at LOG.trace
?
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.
You are right (though I suppose it doesn't matter much because of the if-check.. but let me fix. Thanks for review.
🎊 +1 overall
This message was automatically generated. |
} else { | ||
LOG.info("Could not delete {} under {}; will retry. If it keeps happening, enable" + | ||
"TRACE-level logging and quote the exception when asking on mailing list.", | ||
type, dir, ioe.getMessage()); |
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: space between enable
and TRACE-level
.
One more concern. I may be wrong in this. ioe.getMessage()
will be treated as string and not Throwable. Since we are using string formatter, we need another {}
formatter to add exception's message to log.
@saintstack Could you please verify that log is printed as expected ? Thank you !
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 is a good one. Thank you. Fixed.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
4d27473
to
793c6e7
Compare
…." makes master log unreadable Turn down the amount we log. If you want to see the full exception enable TRACE-level logging.
793c6e7
to
25bab67
Compare
Address the helpful @shahrs87 feedback. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +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.
+1 non-binding. Thank you @saintstack
LOG.trace("Could not delete {} under {}; will retry. If it keeps happening, " + | ||
"quote the exception when asking on mailing list.", type, dir, ioe); | ||
} else { | ||
LOG.info("Could not delete {} under {} because {}; will retry. If it keeps happening, enable" + |
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.
super nit: space between enable and TRACE-level for better readability.
…." makes master log unreadable (#2949) Turn down the amount we log. If you want to see the full exception enable TRACE-level logging. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: shahrs87
…." makes master log unreadable (#2949) Turn down the amount we log. If you want to see the full exception enable TRACE-level logging. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: shahrs87
…." makes master log unreadable
Turn down the amount we log. If you want to see the full exception
enable TRACE-level logging.