cmccabe commented on PR #13407: URL: https://github.com/apache/kafka/pull/13407#issuecomment-1490305931
Thanks for this, @mumrah ! I only had a chance to skim this morning (it's 6 AM here) but I figured I would give some feedback as early as I can. I'm thinking that maybe it makes more sense to put the ZK migration state enum in FeatureControlManager? It's kind of annoying to have a whole separate object just for a single enum. We've been accumulating a lot of small "image" objects and it's kind of unwieldy. If we know that we're not adding more migration state later, I think I'd prefer to just have the migration state in FeatureControlManager. That also makes the PR smaller of course :) I don't like the idea of ZkMigrationBootstrap.java, to be honest. We can just add some code that emits the correct migration state record in QuorumController's becomeActive function, right? This should be doable regardless of whether we're in 3.4-IV0 or 3.5-IV0 since the migration state record existed in 3.4. Also, I don't think we need to emit a migration state record if the log is non-empty, since in that case we know the ZkMigrationState value must be NONE. And we already need to treat no record as NONE since that is necessary for all IBPs prior to 3.4-IV0. -- 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
