MINOR: Fix log format in AbstractCoordinator#10247
MINOR: Fix log format in AbstractCoordinator#10247dengziming wants to merge 1 commit intoapache:trunkfrom
Conversation
dengziming
changed the title
|
@ableegoldman , Hi, PTAL. |
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@dengziming Thanks for your patch. LGTM
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@dengziming Sorry that I have some comments when making another look.
Could you fix another similar issue (see https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/security/kerberos/KerberosError.java#L100)?
| @Override | ||
| public void onFailure(RuntimeException e, RequestFuture<Void> future) { | ||
| log.debug("FindCoordinator request failed due to {}", e); | ||
| log.debug("FindCoordinator request failed due to {}", e.getMessage()); |
There was a problem hiding this comment.
How about using e.toString()? It shows the class name of exception. I feel it is useful also.
There was a problem hiding this comment.
It's not clear what was intended here. Is the stacktrace useful or not? If the {} is removed, then the stacktrace would be printed. If we make the change in this PR, we don't include the stacktrace. @hachikuji Thoughts since you have done a lot of coordinator debugging?
There was a problem hiding this comment.
@guozhangwang has a PR open in which he fixes this on the side. See https://github.com/apache/kafka/pull/10232/files#r584478802
There was a problem hiding this comment.
Thank you, the #10232 LGTM, will close this pr.
|
@chia7712 Thank you,There are 5 overloadings of log.debug, the code in your comment isn't the same with mine, WDYT? |
4 participants
More detailed description of your change
Accidentally saw a log:
I fixed it to:
I think the author wants to call
Logger.debug(String format, Object arg), but calledLogger.debug(String msg, Throwable t). In fact, there are 5 overloadingsLogger.debug()so we need to be careful.Summary of testing strategy (including rationale)
test locally
Committer Checklist (excluded from commit message)