You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/08/26 17:11:48 UTC
[GitHub] [kafka] mumrah opened a new pull request #11266: KAFKA-13233 Log zkVersion in more places
mumrah opened a new pull request #11266:
URL: https://github.com/apache/kafka/pull/11266
When debugging issues with partition state, it's very useful to know the zkVersion that was written. This patch adds the zkVersion of LeaderAndIsr in a few more places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11266:
URL: https://github.com/apache/kafka/pull/11266#discussion_r696859115
##########
File path: core/src/main/scala/kafka/controller/PartitionStateMachine.scala
##########
@@ -437,6 +437,11 @@ class ZkPartitionStateMachine(config: KafkaConfig,
}
}
+ updatesToRetry.foreach { partition =>
Review comment:
Maybe it's worthwhile adding `isDebugEnabled` around these loops?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #11266:
URL: https://github.com/apache/kafka/pull/11266#issuecomment-906622164
> A side question is whether any of the zk updates in ReplicaStateMachine and PartitionStateMachine need similar enhancements?
Added a debug of the partitions to retry in those classes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah merged pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #11266:
URL: https://github.com/apache/kafka/pull/11266
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11266:
URL: https://github.com/apache/kafka/pull/11266#discussion_r696875779
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -2344,17 +2344,17 @@ class KafkaController(val config: KafkaConfig,
debug(s"ISR for partition $partition updated to [${updatedIsr.isr.mkString(",")}] and zkVersion updated to [${updatedIsr.zkVersion}]")
partitionResponses(partition) = Right(updatedIsr)
Some(partition -> updatedIsr)
- case Left(error) =>
- warn(s"Failed to update ISR for partition $partition", error)
- partitionResponses(partition) = Left(Errors.forException(error))
+ case Left(err) =>
Review comment:
nit: I didn't realize this was an exception on first review. How about just `e`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #11266:
URL: https://github.com/apache/kafka/pull/11266#issuecomment-907396380
Cherry-picked to:
* 2.8 as 16ef2ae9e474
* 2.7 as 39a52ee91d7a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11266: KAFKA-13233 Log zkVersion in more places
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11266:
URL: https://github.com/apache/kafka/pull/11266#discussion_r696830865
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -2345,14 +2345,14 @@ class KafkaController(val config: KafkaConfig,
partitionResponses(partition) = Right(updatedIsr)
Some(partition -> updatedIsr)
case Left(error) =>
- warn(s"Failed to update ISR for partition $partition", error)
+ this.error(s"Failed to update ISR for partition $partition", error)
partitionResponses(partition) = Left(Errors.forException(error))
None
}
}
badVersionUpdates.foreach(partition => {
- debug(s"Failed to update ISR for partition $partition, bad ZK version")
+ info(s"Failed to update ISR to ${adjustedIsrs(partition)} for partition $partition, bad ZK version.")
Review comment:
nit: while we're in here, maybe we can update the loop with the more conventional pattern:
```scala
badVersionUpdates.foreach { partition =>
```
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -2345,14 +2345,14 @@ class KafkaController(val config: KafkaConfig,
partitionResponses(partition) = Right(updatedIsr)
Some(partition -> updatedIsr)
case Left(error) =>
- warn(s"Failed to update ISR for partition $partition", error)
+ this.error(s"Failed to update ISR for partition $partition", error)
Review comment:
nit: maybe we could rename the variable to `err` and drop the `this.`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org