-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
MINOR: Fixed way how logging methods are used for having a consistent one #3419
Conversation
Cc @dguy |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
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 patch @ppatierno. I have just one comment.
@@ -72,7 +72,7 @@ public void makeReady(final Map<InternalTopicConfig, Integer> topics) { | |||
streamsKafkaClient.createTopics(topicsToBeCreated, replicationFactor, windowChangeLogAdditionalRetention, metadata); | |||
return; | |||
} catch (StreamsException ex) { | |||
log.warn("Could not create internal topics: " + ex.getMessage() + " Retry #" + i); | |||
log.warn("Could not create internal topics. Retry #{} :", i, ex); |
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.
We don't really care about the stack trace here or below, just the message. We know the StreamsException
is generated from this class and the message is enough to identify the problem. Further, these are done in a retry loop so can become quite verbose if failed multiple times
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.
@dguy just fixed even in the getNumPartitions
method. thanks !
Fixed avoid the entire stack trace instead of the message only
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 @ppatierno. LGTM
5a3d698
to
2ef61eb
Compare
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
In the stream library there are few cases where we don't leverage on logging methods features (i.e. using {} placeholder instead of string concatenation or passing the exception variable)