KAFKA-7906: Improve logging for failed leader elections #8176
Conversation
…tead of within `state.changed.logger`
agam
force-pushed
the
improve-failed-election-logging
branch
from
45e9557
to
cb25ec3
Compare
| "" | ||
| } | ||
| logger.error(s"Controller $controllerId epoch ${controllerContext.epoch} failed to " | ||
| + s" change state for partition $partition from ${partitionState(partition)} " |
There was a problem hiding this comment.
nit: extra space at the start
| } else { | ||
| "" | ||
| } | ||
| logger.error(s"Controller $controllerId epoch ${controllerContext.epoch} failed to " |
There was a problem hiding this comment.
The controllerId is already included in logIdent. Perhaps we could add controller epoch to logIdent as well and remove it from log lines where it appears unnecessarily.
There was a problem hiding this comment.
Done. Went through the other log messages in the file, the rest are passed through StateChangeLogger (which has its own logIdent, so the id and epoch are useful there)
| } | ||
| logger.error(s"Controller $controllerId epoch ${controllerContext.epoch} failed to " | ||
| + s" change state for partition $partition from ${partitionState(partition)} " | ||
| + s"to $OnlinePartition $reasonMsg", e) |
There was a problem hiding this comment.
With this change we'll actually see the exception message twice since we are still including it in the call to error. Would it be worthwhile to check for StateChangeFailedException separately? Perhaps we can print the stack trace only when it is an unexpected exception type.
There was a problem hiding this comment.
Added separate cases for StateChangeFailedException and others.
…failure message depending on type of exception
| logFailedStateChange(partition, partitionState(partition), OnlinePartition, e) | ||
| // Extract failure message, if present, to append to error log. | ||
| val reasonMsg = if (e.isInstanceOf[StateChangeFailedException]) { | ||
| s", reason: ${e.getMessage}" |
There was a problem hiding this comment.
Wouldn't it be simpler to move this into the check below?
| } else { | ||
| "" | ||
| } | ||
| val failMsg = s"Controller failed to change state for partition $partition from ${partitionState(partition)} to $OnlinePartition" |
There was a problem hiding this comment.
nit: since the log identity already starts with "Controller," I think we can leave it out and start from "Failed."
| "" | ||
| } | ||
| val failMsg = s"Controller failed to change state for partition $partition from ${partitionState(partition)} to $OnlinePartition" | ||
| if (e.isInstanceOf[StateChangeFailedException]) { |
There was a problem hiding this comment.
Using a match is considered better style. For example:
match {
case e: StateChangeFailedException => ...
case _ => ...
}
There was a problem hiding this comment.
Thanks, did the equivalent
e match {
case _:StateChangeFailedException => ...
case _ => ...
}
| // Extract failure message, if present, to append to error log. | ||
| val failMsg = s"Failed to change state for partition $partition from ${partitionState(partition)} to $OnlinePartition" | ||
| e match { | ||
| case _:StateChangeFailedException => logger.error(s"$failMsg, reason: ${e.getMessage}") |
There was a problem hiding this comment.
nit: add space after colon. Also, you can use error instead of logger.error
kafka.controllerclass hierarchy instead of withinstate.changed.loggerCommitter Checklist (excluded from commit message)