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/24 16:12:05 UTC

[GitHub] [kafka] splett2 opened a new pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

splett2 opened a new pull request #11255:
URL: https://github.com/apache/kafka/pull/11255


   ### What
   The controller can skip sending `updateMetadataRequest` during the broker failure callback if there are offline partitions and the deleted brokers don't host any partitions. Looking at the logic, I'm not sure why the if check is checking for partitionsWithOfflineLeader. This seems like a bug which may mean we're sending additional `updateMetadataRequests` on broker shutdowns.
   
   ### Testing
   Added an integration test for the failure scenario. The controller integration test suite passes locally with my change.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700643818



##########
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:
       Yes, your understanding is correct.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r696141192



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       I'm still don't follow why the check for `partitionsWithOfflineLeader` is necessary.
   
   If the broker doesn't host any replicas, then we need to send out `UpdateMetadataRequest` because the partition state machines will not trigger any control requests.
   
   If the broker does host replicas, then shouldnt `replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq, OfflineReplica)`
   handle sending out UpdateMetadataRequest to propagate the offline broker? `partitionsWithOfflineLeader.isEmpty` seems redundant.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
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. 




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r701230149



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -628,9 +628,11 @@ 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 partition has changed leader or ISR, no UpdateMetadataRequest is sent through PartitionStateMachine
+    // and ReplicaStateMachine. In that case, we want to send an UpdateMetadataRequest explicitly to
+    // propagate the information about the new offline brokers.

Review comment:
       With this comment, we could just remove the comment on the next line.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r696141192



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       I don't follow why the check for `partitionsWithOfflineLeader` is necessary.
   
   If the broker doesn't host any replicas, then we need to send out `UpdateMetadataRequest` because the partition state machines will not trigger any control requests. This is handled by the check for `newOfflineReplicas.isEmpty`
   
   If the broker does host replicas, then shouldnt `replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq, OfflineReplica)`
   handle sending out UpdateMetadataRequest to propagate the offline broker? Maybe we would also need a check if `newOfflineReplicasNotForDeletion` is empty as well, but `partitionsWithOfflineLeader.isEmpty` seems redundant and fairly coarse.




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700661885



##########
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 have ISR change, inform brokers of the offline brokers

Review comment:
       How about changing the comments to  the following?
   
   If no partition has changed leader or ISR, no UpdateMetadataReuqest is sent through PartitionStateMachine and ReplicaStateMachine. In that case, we want to send an UpdateMetadataReuqest explicitly to propagate the information about the new offline brokers.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r696141192



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       I don't follow why the check for `partitionsWithOfflineLeader` is necessary.
   
   If the broker doesn't host any replicas, then we need to send out `UpdateMetadataRequest` because the partition state machines will not trigger any control requests. This is handled by the check for `newOfflineReplicas.isEmpty`
   
   If the broker does host replicas, then shouldnt `replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq, OfflineReplica)`
   handle sending out UpdateMetadataRequest to propagate the offline broker? `partitionsWithOfflineLeader.isEmpty` seems redundant.




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700644408



##########
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

Review comment:
       If no partitions are affected => If no partitions have leader change and thus no UpdateMetadataRequest is sent

##########
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 (newOfflineReplicasNotForDeletion.isEmpty && onlineStateChangeResults.values.forall(_.isLeft)) {

Review comment:
       There is no need to test `newOfflineReplicasNotForDeletion.isEmpty` since it's covered by the second condition.




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r696035745



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       Good catch. There are two cases where we need to send UpdateMetadataRequest.
   
   1. If the broker doesn't host any replicas. This is the new case that you identified since we won't send any UpdateMetadataRequest in PartitionStateMachine and we want to propagate the offline broker to other brokers.
   2. If partitionsWithOfflineLeader is not empty, partitionStateMachine.triggerOnlinePartitionStateChange() would send UpdateMetadataRequest as part of the transitioning of those partitions to online. So, we don't need to send UpdateMetadataRequest again. If partitionsWithOfflineLeader is empty, we do want to send UpdateMetadataRequest to propagate the offline broker.
   
   So, it seem that the check should be if (newOfflineReplicas.isEmpty || partitionsWithOfflineLeader.isEmpty). It would be useful to adjust the comment above accordingly.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700657822



##########
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

Review comment:
       is it only for leader change? any ISR change in `replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq, OfflineReplica)` 
   will also send an UpdateMetadataRequest. Maybe it should be if no partitions have ISR change?




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698652526



##########
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:
       On second thought, I think this change still doesn't cover all the cases. The main issue is that in partitionStateMachine.triggerOnlinePartitionStateChange(), we only send updateMetadataRequest for partitions with successful leader change. If no partition has successful leader change, no updateMetadataRequest will be sent. The reason could be that newOfflineReplicas is empty, but it could also be that the leader can't be elected for other reasons (e.g. no replica is live). 
   
   In PartitionStateMachine.doElectLeaderForPartitions(), we have 3 categories of partitions, finishedUpdates, failedElections and updatesToRetry. Perhaps we could return finishedUpdates all the way to partitionStateMachine.triggerOnlinePartitionStateChange() and force a send of updateMetadataRequest if finishedUpdates is empty.
    




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698693107



##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(etc)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```

##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(etc, etc)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698849242



##########
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:
       I think we only need to check the return result of `partitionStateMachine.triggerOnlinePartitionStateChange()`. `partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)` doesn't generate any  updateMetadataRequest.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r697707732



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       @junrao 
   did you mean `newOfflineReplicas.isEmpty || (partitionsWithOfflineLeader.isEmpty && newOfflineReplicasNotForDeletion.isEmpty)?




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700656961



##########
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 (newOfflineReplicasNotForDeletion.isEmpty && onlineStateChangeResults.values.forall(_.isLeft)) {

Review comment:
       how so?
   I thought that `partitionStateMachine.triggerOnlinePartitionStateChange()` handles partition transitions from offline/new => online, whereas
   `replicaStateMachine.handleStateChanges(newOfflineReplicasNotForDeletion.toSeq, OfflineReplica)` handles the replicas on the current broker going offline. I would have thought that these are sort of orthogonal, and that we would need to check both.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698693107



##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(_)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```

##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(etc)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```

##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(etc, etc)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r696210968



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       Good point. Basically, if there is at least one partition that has gone through each `partitionStateMachine.triggerOnlinePartitionStateChange()` or ` replicaStateMachine.handleStateChanges()`, there is no need to send UpdateMetadataRequest since we broadcast metadata to every broker in each call. If partitionsWithOfflineLeader is empty but partitionsWithOfflineLeader is not empty, there is no need to send UpdateMetadataRequest. So, the check probably should be sth like
   
   ` if (newOfflineReplicas.isEmpty || (partitionsWithOfflineLeader.isEmpty && partitionsWithOfflineLeader.isEmpty))`




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r697714522



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -630,7 +630,7 @@ class KafkaController(val config: KafkaConfig,
 
     // If replica failure did not require leader re-election, inform brokers of the offline brokers
     // Note that during leader re-election, brokers update their metadata
-    if (partitionsWithOfflineLeader.isEmpty) {
+    if (newOfflineReplicas.isEmpty) {

Review comment:
       @splett2 : Yes. Sorry for the confusion.




-- 
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] junrao merged pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao merged pull request #11255:
URL: https://github.com/apache/kafka/pull/11255


   


-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
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 **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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698693107



##########
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:
       I think that makes sense. Do we need to do something similar for both `partitionStateMachine` calls?
   `handleStateChanges` already returns a `Map[TopicPartition, Either[Throwable, LeaderAndIsr]]` so it seems like we could just look to see if there are any values with `LeaderAndIsr` and that would imply that we sent out `updateMetadataRequest` as part of Online/Offline state changes. and likewise, we can have `triggerOnlinePartitionStateChange` return the same results.
   
   I am thinking something along the lines of:
   ```
       // trigger OfflinePartition state for all partitions whose current leader is one amongst the newOfflineReplicas
       val offlineStateChangeResults = partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)
       // trigger OnlinePartition state changes for offline or new partitions
       val onlineStateChangeResults = partitionStateMachine.triggerOnlinePartitionStateChange()
   
   ...
       val leaderElectionSucceeded = (offlineStateChangeResults ++ onlineStateChangeResults).find(_)
       // (!leaderElectionSucceeded && newOfflineReplicasForDeletion.isEmpty) {
         sendUpdateMetadataRequest(controllerContext.liveOrShuttingDownBrokerIds.toSeq, Set.empty)
       }
   ```




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r700660131



##########
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 (newOfflineReplicasNotForDeletion.isEmpty && onlineStateChangeResults.values.forall(_.isLeft)) {

Review comment:
       Sorry, you are right. I thought newOfflineReplicasNotForDeletion is newOfflineReplicas. So this is fine.




-- 
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] splett2 commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r701247464



##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -628,9 +628,11 @@ 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 partition has changed leader or ISR, no UpdateMetadataRequest is sent through PartitionStateMachine
+    // and ReplicaStateMachine. In that case, we want to send an UpdateMetadataRequest explicitly to
+    // propagate the information about the new offline brokers.

Review comment:
       good point thanks.




-- 
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] junrao commented on a change in pull request #11255: KAFKA-13225: Controller skips sending UpdateMetadataRequest when no change in partition state.

Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11255:
URL: https://github.com/apache/kafka/pull/11255#discussion_r698652526



##########
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:
       On second thought, I think this change still doesn't cover all the cases. The main issue is that in partitionStateMachine.triggerOnlinePartitionStateChange(), we only send updateMetadataRequest for partitions with successful leader change. If no partition has successful leader change, no updateMetadataRequest will be sent. The reason could be that newOfflineReplicas is empty, but it could also be that the leader can't be elected for other reasons (e.g. no replica is live). 
   
   In PartitionStateMachine.doElectLeaderForPartitions(), we have 3 categories of partitions, finishedUpdates, failedElections and updatesToRetry. Perhaps we could return finishedUpdates all the way to partitionStateMachine.triggerOnlinePartitionStateChange() and force a send of updateMetadataRequest if finishedUpdates is empty.
    

##########
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:
       I think we only need to check the return result of `partitionStateMachine.triggerOnlinePartitionStateChange()`. `partitionStateMachine.handleStateChanges(partitionsWithOfflineLeader.toSeq, OfflinePartition)` doesn't generate any  updateMetadataRequest.




-- 
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