[GitHub] [kafka] mumrah commented on a diff in pull request #13631: KAFKA-14909: check zkMigrationReady tag before migration

["mumrah (via GitHub)" <gi...@apache.org>]

mumrah commented on code in PR #13631:
URL: https://github.com/apache/kafka/pull/13631#discussion_r1175362654


##########
metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java:
##########
@@ -134,8 +139,7 @@ private void initializeMigrationState() {
     }
 
     private boolean isControllerQuorumReadyForMigration() {
-        // TODO implement this
-        return true;
+        return this.apiVersions.isAllNodeZkMigrationReady();

Review Comment:
   When we check to see if the quorum nodes are ready, it would be useful to print an error log showing which controllers were not ready here (similar to what we do in `areZkBrokersReadyForMigration`). 
   
   
   
   



##########
clients/src/main/java/org/apache/kafka/clients/ApiVersions.java:
##########
@@ -64,4 +64,8 @@ public synchronized byte maxUsableProduceMagic() {
         return maxUsableProduceMagic;
     }
 
+    // check if all nodes are ZK Migration ready
+    public boolean isAllNodeZkMigrationReady() {

Review Comment:
   Can we move this logic into QuorumFeatures? That would allow us to verify that all the expected controllers are present in the ApiVersions (in addition to checking the `zkMigrationEnabled` flag)



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