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
KAFKA-15039: Reduce logging level to trace in PartitionChangeBuilder.… #13780
Conversation
topicId, | ||
partitionId, | ||
electionResult.node, | ||
electionResult.unclean ? "an unclean" : "a clean" |
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.
I wonder if it would make sense to log unclean elections at a higher level?
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.
I don't think it is possible to do so since we don't know the result of the election anywhere else. Unclean leader election may be enabled, for example, but it may not ever occur. It is only here that we know. I added additional logic to ensure that we will always log an unclean election at the WARN level.
// so only log clean elections at TRACE level; | ||
// log unclean elections at WARN level since doing so can lead to data loss. | ||
if (electionResult.unclean) { | ||
log.warn( |
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.
How about INFO level? Unclean election is "expected behavior" if it is enabled or requested specifically.
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.
Sure, sounds good. Will adjust.
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 PR, @rondagostino .
The trace guard is not needed here since we're not doing computation. (log4j is supposed to not build the string unless it needs to perform the log). Also, these log messages really should be one or two lines of vertical space (I realize this is an existing issue).
LGTM once that's addressed
Test failures are unrelated. |
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.
LGTM
…tryElection()
A CPU profile in a large cluster showed PartitionChangeBuilder.tryElection() taking significant CPU due to logging. Decrease the logging statements in that method from debug level to trace to mitigate the impact of this CPU hog under normal operations.
Committer Checklist (excluded from commit message)