Hi, David, Thanks for the reply. No other comments from me.
Jun On Tue, Nov 29, 2022 at 2:57 PM David Arthur <david.art...@confluent.io.invalid> wrote: > Jun, > > 51. You're right, I missed that in the latest update. It's fixed now. > > 54. I was thinking we could update meta.properties to v1 as the brokers > were being restarted in KRaft mode without the migration config set. > However, as you mentioned, it is still possible to downgrade even then (as > long as the controller has not exited dual-write mode). Upgrading the > meta.properties after seeing the final ZkMigrationRecord sounds like a good > idea to me. I've updated the KIP to include this detail under > "Meta.Properties" section. > > 58. Yes, the metadata migration from ZK to KRaft will not migrate the > contents of /brokers. > > Thanks, > David > > On Tue, Nov 29, 2022 at 4:50 PM Jun Rao <j...@confluent.io.invalid> wrote: > > > Hi, David, > > > > Thanks for the reply. > > > > 51. Is this reflected in the KIP? It seems that ZkMigrationState still > has > > the None value. > > > > 54. Supporting both v0 and v1 indefinitely in a KRaft broker could be a > bit > > confusing and may complicate future upgrades. Another approach is to let > > KRaft broker write the v1 meta.properties after the KRaft controller > exits > > the dual write mode. We could extend ZkMigrationRecord to 3 states like > > migration, dualWrite and KRaftOnly. Once a broker sees KRaftOnly, it will > > write meta.properties in v1 format. At that point, downgrade could cause > > metadata loss and require manual work. Will that work? > > > > 58. When copying metadata from ZK to KRaft, I guess we will ignore broker > > registration since the KRaft controller has already generated a > > BrokerRegistrationRecord based on BrokerRegistrationRequest? > > > > Thanks, > > > > Jun > > > > On Tue, Nov 29, 2022 at 7:14 AM David Arthur > > <david.art...@confluent.io.invalid> wrote: > > > > > Jun, Thanks for the comments. Igor, please see 54 below for some > > additional > > > discussion on the meta.properties > > > > > > 50.1 Yes, that field name sounds fine to me. > > > > > > 50.2 Ok, I'll add something to the KIP under the Controller section. To > > > your other question, NoOpRecords are used as part of our liveness check > > for > > > the quorum. It doesn't produce any metadata really, so I don't think it > > > causes any harm to let it happen before the migration. KIP-835 has the > > > details on the NoOpRecords > > > > > > 54. Colin and I discussed the meta.properties issue last night. How > about > > > we simply let the KRaft broker accept v0 or v1 meta.properties. At this > > > point, the two versions have the same contents, but different field > > names. > > > By leaving the meta.properties intact, we can simplify the downgrade > > > process. If we care to, we could rewrite meta.properties once a broker > is > > > restarted after the migration is finalized (migration config disabled). > > > > > > 57. If a ZK broker can't send a BrokerRegistrationRequest because the > > > quorum is unavailable, it should just continue operating normally. > Once a > > > leader is available, the broker will send the registration and start > > > heart-beating. Unlike KRaft mode, we won't block startup on a > successful > > > BrokerRegistration response. Concretely, BrokerLifecycleManager will > keep > > > trying to contact the quorum in its own thread until the > > > BrokerToChannelManager gets a controller ID from KafkaRaftManager. This > > > shouldn't interfere with other ZK broker activity. > > > > > > -David > > > > > > > > > > > > > -- > > > -David > > > > > > > > -- > -David >
