Thanks for the reply. Yes, I think adding a section in Rejected Alternatives to explain the rationale why we don't support combined mode upgrade in this KIP is helpful.
Thank you. Luke On Wed, Nov 30, 2022 at 8:47 AM Jun Rao <j...@confluent.io.invalid> wrote: > 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 > > >
