cmccabe commented on code in PR #13407: URL: https://github.com/apache/kafka/pull/13407#discussion_r1173089106
########## metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java: ########## @@ -222,7 +245,7 @@ private boolean isValidStateChange(MigrationDriverState newState) { private void transitionTo(MigrationDriverState newState) { if (!isValidStateChange(newState)) { - log.error("Error transition in migration driver from {} to {}", migrationState, newState); + log.error("Invalid transition in migration driver from {} to {}", migrationState, newState); Review Comment: I'm confused about this function: 1. Shouldn't the transition to UNINITIALIZED being invalid be covered by isValidStateChange returning false if you pass it newState = UNINITIALIZED ? 2. The switch statement below throws an exception on invalid state change, whereas the initial check just logs a message at `error`. The exception seems more reasonable? 3. the switch has a case for INACTIVE that just does nothing? ``` switch (newState) { ... case INACTIVE: // Any state can go to INACTIVE. break; } ``` Just leave out the case for `INACTIVE` since it's a no-op? (And maybe the whole switch, see above.) -- 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