Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KAFKA-7906: Improve logging for failed leader elections #8176

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

KAFKA-7906: Improve logging for failed leader elections #8176

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
794327b
Log election errors within the `kafka.controller` class hierarchy ins…
Feb 26, 2020
cb25ec3
Log failure message when present
Feb 26, 2020
0a5f651
Review: moved controller-epoch to logident, added separate cases for …
Mar 11, 2020
25f6a30
nit: formatting error message
Mar 11, 2020
d2e1ef2
Review: idiomatic style, improve log message
Mar 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions core/src/main/scala/kafka/controller/PartitionStateMachine.scala
View file
Open in desktop
Expand Up @@ -131,7 +131,7 @@ class ZkPartitionStateMachine(config: KafkaConfig,
extends PartitionStateMachine(controllerContext) {

private val controllerId = config.brokerId
this.logIdent = s"[PartitionStateMachine controllerId=$controllerId] "
this.logIdent = s"[PartitionStateMachine controllerId=$controllerId controllerEpoch=${controllerContext.epoch}] "

/**
* Try to change the state of the given partitions to the given targetState, using the given
Expand Down Expand Up @@ -337,7 +337,18 @@ class ZkPartitionStateMachine(config: KafkaConfig,

finished.foreach {
case (partition, Left(e)) =>
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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to move this into the check below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} else {
""
}
val failMsg = s"Controller failed to change state for partition $partition from ${partitionState(partition)} to $OnlinePartition"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since the log identity already starts with "Controller," I think we can leave it out and start from "Failed."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (e.isInstanceOf[StateChangeFailedException]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a match is considered better style. For example:

match {
  case e: StateChangeFailedException => ...
  case _ => ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, did the equivalent

e match {
  case _:StateChangeFailedException => ...
  case _ => ...
}

logger.error(s"$failMsg: $reasonMsg")
} else {
logger.error(s"$failMsg: unknown exception: ", e)
}
case (_, Right(_)) => // Ignore; success so no need to log failed state change
}

Expand Down