kevin-wu24 commented on code in PR #23338:
URL: https://github.com/apache/kafka/pull/23338#discussion_r3917806513


##########
metadata/src/main/java/org/apache/kafka/controller/ActivationRecordsGenerator.java:
##########
@@ -182,10 +185,17 @@ static ControllerResult<Void> recordsForNonEmptyLog(
                         // This can happen if controller leadership transfers 
to a controller with migrations enabled
                         // after another controller had finalized the 
migration. For example, during a rolling restart
                         // of the controller quorum during which the migration 
config is being set to false.
-                        logMessageBuilder
-                            .append("Completing the ZK migration since this 
controller was configured with ")
-                            .append("'zookeeper.metadata.migration.enable' set 
to 'false'. ");
-                        
records.add(ZkMigrationState.POST_MIGRATION.toRecord());
+                        if (!stillZkRegisteredBrokerIds.isEmpty()) {
+                            logMessageBuilder
+                                .append("Cannot complete ZK migration because 
the following broker(s) are still registered as ZK brokers: ")
+                                .append(new 
TreeSet<>(stillZkRegisteredBrokerIds))

Review Comment:
   What about the following?
   ```suggestion
                                   .append(stillZkRegisteredBrokerIds)
   ```
   



##########
metadata/src/main/java/org/apache/kafka/controller/ActivationRecordsGenerator.java:
##########
@@ -182,10 +185,17 @@ static ControllerResult<Void> recordsForNonEmptyLog(
                         // This can happen if controller leadership transfers 
to a controller with migrations enabled
                         // after another controller had finalized the 
migration. For example, during a rolling restart
                         // of the controller quorum during which the migration 
config is being set to false.
-                        logMessageBuilder
-                            .append("Completing the ZK migration since this 
controller was configured with ")
-                            .append("'zookeeper.metadata.migration.enable' set 
to 'false'. ");
-                        
records.add(ZkMigrationState.POST_MIGRATION.toRecord());
+                        if (!stillZkRegisteredBrokerIds.isEmpty()) {
+                            logMessageBuilder
+                                .append("Cannot complete ZK migration because 
the following broker(s) are still registered as ZK brokers: ")
+                                .append(new 
TreeSet<>(stillZkRegisteredBrokerIds))
+                                .append(". Restart these brokers in KRaft mode 
before finalizing the migration. ");

Review Comment:
   ```suggestion
                                   .append(". Restart these brokers in KRaft 
mode before restarting the controllers to finalize the migration. ");
   ```



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to