-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ZOOKEEPER-3001: Incorrect log message when try to delete container node #492
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
Conversation
| try { | ||
| LOG.info("Attempting to delete candidate container: %s", | ||
| containerPath); | ||
| LOG.info(String.format("Attempting to delete candidate container: %s", |
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.
As this a log message you could use
LOG.info("Attempting to delete candidate container: {}", container Path);
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.
to use the same logging style as the other code.
LOG.error(String.format("Could not delete container: %s" ,
containerPath), e);
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 for @eribeiro
We already use that form at various places in the codebase, so feel free to use the better one. Existing String.formats should be refactored in the long term, but that probably will happen in small refactorings like this.
anmolnar
left a comment
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 for the contribution @sel-fish
It seems like a very nice catch.
| try { | ||
| LOG.info("Attempting to delete candidate container: %s", | ||
| containerPath); | ||
| LOG.info(String.format("Attempting to delete candidate container: %s", |
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 for @eribeiro
We already use that form at various places in the codebase, so feel free to use the better one. Existing String.formats should be refactored in the long term, but that probably will happen in small refactorings like this.
anmolnar
left a comment
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 lgtm
|
+1 lgtm |
Missing `String.format`. Author: sel-fish <fqlgy@hotmail.com> Reviewers: Edward Ribeiro <edward.ribeiro@gmail.com>, Andor Molnár <andor@cloudera.com>, Michael Han <hanm@apache.org>, maoling <maoling199210191@sina.com> Closes #492 from sel-fish/ZOOKEEPER-3001 (cherry picked from commit 8cfca3a) Signed-off-by: Michael Han <hanm@apache.org>
Missing `String.format`. Author: sel-fish <fqlgy@hotmail.com> Reviewers: Edward Ribeiro <edward.ribeiro@gmail.com>, Andor Molnár <andor@cloudera.com>, Michael Han <hanm@apache.org>, maoling <maoling199210191@sina.com> Closes apache#492 from sel-fish/ZOOKEEPER-3001
Missing `String.format`. Author: sel-fish <fqlgy@hotmail.com> Reviewers: Edward Ribeiro <edward.ribeiro@gmail.com>, Andor Molnár <andor@cloudera.com>, Michael Han <hanm@apache.org>, maoling <maoling199210191@sina.com> Closes apache#492 from sel-fish/ZOOKEEPER-3001
Missing
String.format.