Hi, David, Thanks for the KIP. +1
Jun On Tue, Nov 22, 2022 at 3:43 PM Jason Gustafson <ja...@confluent.io.invalid> wrote: > Thanks, +1 from me. I suspect we might be in for at least one surprise with > the re-implemented controller RPCs, but I agree the alternative has risks > as well. > > Best, > Jason > > On Mon, Nov 14, 2022 at 12:00 PM Colin McCabe <cmcc...@apache.org> wrote: > > > On Fri, Nov 11, 2022, at 08:59, David Arthur wrote: > > > Thanks, Colin. > > > > > >> never start an upgrade without first verifying the quorum > configuration > > on the ZK-based brokers > > > > > > I agree that this is a pretty big benefit. I could imagine debugging > > > and fixing connection problems mid-migration would be a big pain. > > > Especially if you had some brokers correctly configured, and others > > > not. > > > > > > Adding a heartbeat raises some questions about what to do if a broker > > > goes into a bad state, or stops heartbeating, during a migration. > > > However, I think the same is true for a registration based approach, > > > so maybe it's not an increase in net complexity. > > > > > > > Hi David, > > > > Yeah. I think the goal should be for the set of heartbeaters to match the > > set of broker registrations under /brokers > > > > Obviously, people could add or remove brokers after the upgrade has > begun, > > but that's unavoidable, I think. We can at least ensure that at the time > we > > enter upgrade, all the brokers are ready. > > > > > I've replaced the ZK registration section with a new RPC and brief > > > description. Please take a look. > > > > > > > Thanks, David. With these changes it LGTM to me. > > > > +1 (binding) > > > > Colin > > > > > Thanks! > > > David > > > > > > On Wed, Nov 9, 2022 at 5:46 PM Colin McCabe <cmcc...@apache.org> > wrote: > > >> > > >> Hi David, > > >> > > >> Thanks for the response. Replies inline. > > >> > > >> On Wed, Nov 9, 2022, at 08:17, David Arthur wrote: > > >> > Colin > > >> > > > >> >> Maybe zk.metadata.migration.enable ? > > >> > > > >> > Done. I went with "zookeeper.metadata.migration.enable" since our > > >> > other ZK configs start with "zookeeper.*" > > >> > > > >> >> SImilarly, for MigrationRecord: can we rename this to > > ZkMigrationStateRecord? Then change MigrationState -> ZkMigrationState. > > >> > > > >> > Sure > > >> > > > >> >> With ZkMigrationStateRecord, one thing to keep in mind here is that > > we will eventually compact all the metadata logs into a snapshot. That > > snapshot will then have to keep alive the memory of the old migration. So > > it is not really a matter of replaying the old metadata logs (probably) > but > > a matter of checking to see what the ZkMigrationState is, which I suppose > > could be Optional<ZkMigrationState>. If it's not Optional.empty, we > already > > migrated / are migrating. > > >> > > > >> > Yea, makes sense. > > >> > > > >> >> For the /migration ZNode, is "last_update_time_ms" necessary? I > > thought ZK already tracked this information in the mzxid of the znode? > > >> > > > >> > Yes, Jun pointed this out previously, I missed this update in the > KIP. > > >> > Fixed now. > > >> > > > >> >> It is true that technically it is only needed in UMR, but I would > > still suggest including KRaftControllerId in LeaderAndIsrRequest because > it > > will make debugging much easier. > > >> >> > > >> >> I would suggest not implementing the topic deletion state machine, > > but just deleting topics eagerly when in migration mode. We can implement > > this behavior change by keying off of whether KRaftControllerId is > present > > in LeaderAndIsrRequest. On broker startup, we'll be sent a full > > LeaderAndIsrRequest and can delete stray partitions whose IDs are not as > > expected (again, this behavior change would only be for migration mode) > > >> > > > >> > Sounds good to me. Since this is somewhat of an implementation > detail, > > >> > do you think we need this included in the KIP? > > >> > > >> Yeah, maybe we don't need to go into the delete behavior here. But I > > think the KIP should specify that we have KRaftControllerId in both > > LeaderAndIsrRequest. That will allow us to implement this behavior > > conditionally on zk-based brokers when in dual write mode. > > >> > > >> > > > >> >> For existing KRaft controllers, will > > kafka.controller:type=KafkaController,name=MigrationState show up as 4 > > (MigrationFinalized)? I assume this is true, but it would be good to > spell > > it out. Sorry if this is answered somewhere else. > > >> > > > >> > We discussed using 0 (None) as the value to report for original, > > >> > un-migrated KRaft clusters. 4 (MigrationFinalized) would be for > > >> > clusters which underwent a migration. I have some description of > this > > >> > in the table under "Migration Overview" > > >> > > > >> > > >> I don't feel that strongly about this, but wouldn't it be a good idea > > for MigrationState to have a different value for ZK-based clusters and > > KRaft-based clusters? If you have a bunch of clusters and you take an > > aggregate of this metric, it would be good to get a report of three > numbers: > > >> 1. unupgraded ZK > > >> 2. in progress upgrades > > >> 3. kraft > > >> > > >> I guess we could get that from examining some other metrics too, > > though. Not sure, what do you think? > > >> > > >> >> As you point out, the ZK brokers being upgraded will need to > contact > > the KRaft quorum in order to forward requests to there, once we are in > > migration mode. This raises a question: rather than changing the broker > > registration, can we have those brokers send an RPC to the kraft > controller > > quorum instead? This would serve to confirm that they can reach the > quorum. > > Then the quorum could wait for all of the brokers to check in this way > > before starting the migration (It would know all the brokers by looking > at > > /brokers) > > >> > > > >> > One of the motivations I had for putting the migration details in > the > > >> > broker registration is that it removes ordering constraints between > > >> > the brokers and controllers when starting the migration. If we set > the > > >> > brokers in migration mode before the KRaft quorum is available, they > > >> > will just carry on until the KRaft controller takes over the > > >> > controller leadership and starts sending UMR/LISR. > > >> > > > >> > > >> I agree that any scheme that requires complicated startup ordering is > > not a good idea. I was more thinking about having the zk-based brokers > > periodically send some kind of "I'm here" upgrade heartbeat to the > > controller quorum. > > >> > > >> > I wonder if we could infer reachability of the new KRaft controller > by > > >> > the brokers through ApiVersions? Once taking leadership, the active > > >> > KRaft controller could monitor its NodeApiVersions to see that each > of > > >> > the ZK brokers had reached it. Would this be enough to verify > > >> > reachability? > > >> > > > >> > > >> If the new KRaft controller has already taken leadership, it's too > late > > to do anything about brokers that can't reach it. Unless we want to > > implement automatic rollback to the pre-upgrade state, which sounds quite > > complex and error prone. > > >> > > >> So, to sum up: my proposal was that the ZK based brokers periodically > > (like maybe every 10 seconds) try to contact the quorum with an RPC > > containing their IBP and readiness state. I guess the main negative thing > > is that it would be more network traffic (but probably not significantly > > more.) The main positive thing is that we'd never start an upgrade > without > > first verifying the quorum configuration on the ZK-based brokers. A > neutral > > thing (neither good nor bad, I think) is that we'd have a new RPC rather > > than a znode change. > > >> > > >> best, > > >> Colin > > >> > > >> > > >> > -David > > >> > > > >> > > > >> > On Tue, Nov 8, 2022 at 6:15 PM Colin McCabe <cmcc...@apache.org> > > wrote: > > >> >> > > >> >> Hi David, > > >> >> > > >> >> Looks great. Some questions: > > >> >> > > >> >> I agree with Jun that it would be good to rename > > metadata.migration.enable to something more zk-specific. Maybe > > zk.metadata.migration.enable ? > > >> >> > > >> >> SImilarly, for MigrationRecord: can we rename this to > > ZkMigrationStateRecord? Then change MigrationState -> ZkMigrationState. > > >> >> > > >> >> With ZkMigrationStateRecord, one thing to keep in mind here is that > > we will eventually compact all the metadata logs into a snapshot. That > > snapshot will then have to keep alive the memory of the old migration. So > > it is not really a matter of replaying the old metadata logs (probably) > but > > a matter of checking to see what the ZkMigrationState is, which I suppose > > could be Optional<ZkMigrationState>. If it's not Optional.empty, we > already > > migrated / are migrating. > > >> >> > > >> >> For the /migration ZNode, is "last_update_time_ms" necessary? I > > thought ZK already tracked this information in the mzxid of the znode? > > >> >> > > >> >> It is true that technically it is only needed in UMR, but I would > > still suggest including KRaftControllerId in LeaderAndIsrRequest because > it > > will make debugging much easier. > > >> >> > > >> >> I would suggest not implementing the topic deletion state machine, > > but just deleting topics eagerly when in migration mode. We can implement > > this behavior change by keying off of whether KRaftControllerId is > present > > in LeaderAndIsrRequest. On broker startup, we'll be sent a full > > LeaderAndIsrRequest and can delete stray partitions whose IDs are not as > > expected (again, this behavior change would only be for migration mode) > > >> >> > > >> >> For existing KRaft controllers, will > > kafka.controller:type=KafkaController,name=MigrationState show up as 4 > > (MigrationFinalized)? I assume this is true, but it would be good to > spell > > it out. Sorry if this is answered somewhere else. > > >> >> > > >> >> As you point out, the ZK brokers being upgraded will need to > contact > > the KRaft quorum in order to forward requests to there, once we are in > > migration mode. This raises a question: rather than changing the broker > > registration, can we have those brokers send an RPC to the kraft > controller > > quorum instead? This would serve to confirm that they can reach the > quorum. > > Then the quorum could wait for all of the brokers to check in this way > > before starting the migration (It would know all the brokers by looking > at > > /brokers) > > >> >> > > >> >> best, > > >> >> Colin > > >> >> > > >> >> On Tue, Nov 8, 2022, at 07:23, David Arthur wrote: > > >> >> > Hello everyone, I'd like to start the vote on KIP-866. > > >> >> > > > >> >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-866+ZooKeeper+to+KRaft+Migration > > >> >> > > > >> >> > Thanks! > > >> >> > David Arthur > > >> > > > >> > > > >> > > > >> > -- > > >> > David Arthur > > > > > > > > > > > > -- > > > David Arthur > > >
