splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r699777919
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -628,9 +628,9 @@ class KafkaController(val config: KafkaConfig,
topicDeletionManager.failReplicaDeletion(newOfflineReplicasForDeletion)
}
- // If replica failure did not require leader re-election, inform brokers
of the offline brokers
+ // If no partitions are affected, inform brokers of the offline brokers
// Note that during leader re-election, brokers update their metadata
- if (partitionsWithOfflineLeader.isEmpty) {
+ if (newOfflineReplicas.isEmpty ||
(newOfflineReplicasNotForDeletion.isEmpty &&
partitionsWithOfflineLeader.isEmpty)) {
Review comment:
thanks for the explanation, I think I'm starting to get a better handle
on the intended flow.
Let me know if this seems right:
`partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq,
OfflinePartition)`. This does **not** send LISR/UpdateMetadataRequest.
`partitionStateMachine.triggerOnlinePartitionStateChange()` .This **does**
send LISR/UpdateMetadata
`replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq,
OfflineReplica)` this sends LISR/UpdateMetadata.
So the `sendUpdateMetadataRequest` at the end of onReplicasBecomeOffline
is meant to cover any cases where we did not send out an UpdateMetadataRequest.
So if the previous state machine calls did not send any control requests, we
should send a control request. I don't think we should care about the value of
partitionsWithOfflineLeader because we need to propagate the offline broker to
other brokers' metadata anyway.
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -628,9 +628,9 @@ class KafkaController(val config: KafkaConfig,
topicDeletionManager.failReplicaDeletion(newOfflineReplicasForDeletion)
}
- // If replica failure did not require leader re-election, inform brokers
of the offline brokers
+ // If no partitions are affected, inform brokers of the offline brokers
// Note that during leader re-election, brokers update their metadata
- if (partitionsWithOfflineLeader.isEmpty) {
+ if (newOfflineReplicas.isEmpty ||
(newOfflineReplicasNotForDeletion.isEmpty &&
partitionsWithOfflineLeader.isEmpty)) {
Review comment:
thanks for the explanation, I think I'm starting to get a better handle
on the intended flow.
Let me know if this seems right:
`partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq,
OfflinePartition)`. This does **not** send LISR/UpdateMetadataRequest.
`partitionStateMachine.triggerOnlinePartitionStateChange()` .This **does**
send LISR/UpdateMetadata
`replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq,
OfflineReplica)` this **does** send LISR/UpdateMetadata.
So the `sendUpdateMetadataRequest` at the end of onReplicasBecomeOffline
is meant to cover any cases where we did not send out an UpdateMetadataRequest.
So if the previous state machine calls did not send any control requests, we
should send a control request. I don't think we should care about the value of
partitionsWithOfflineLeader because we need to propagate the offline broker to
other brokers' metadata anyway.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
