Hey everyone, I've updated the KIP with a few items we've discussed so far:
101. Change AllowDowngrade bool to DowngradeType int8 in UpgradeFeatureRequest RPC. I'm wondering if we can kind of "cheat" on this incompatible change since it's not currently in use and totally remove the old field and leave the versions at 0+. Thoughts? 102. Add section on metadata.version initialization. This includes a new option to kafka-storage.sh to let the operator set the initial version 103. Add section on behavior of controllers and brokers if they encounter an unsupported version Thanks for the great discussion so far! -David On Fri, Oct 15, 2021 at 10:23 AM David Arthur <mum...@gmail.com> wrote: > Jose, > > Are you saying that for metadata.version X different software versions >> could generate different snapshots? If so, I would consider this an >> implementation bug, no? The format and content of a snapshot is a >> public API that needs to be supported across software versions. > > > I agree this would be a bug. This is probably a non-issue since we will > recommend brokers to be at the same software version before performing an > upgrade or downgrade. That eliminates the possibility that brokers could > generate different snapshots. > > Hmm. So I think you are proposing the following flow: >> 1. Cluster metadata partition replicas establish a quorum using >> ApiVersions and the KRaft protocol. >> 2. Inactive controllers send a registration RPC to the active controller. >> 3. The active controller persists this information to the metadata log. > > > What happens if the inactive controllers send a metadata.version range >> that is not compatible with the metadata.version set for the cluster? > > > As we discussed offline, we don't need the explicit registration step. > Once a controller has joined the quorum, it will learn about the finalized > "metadata.version" level once it reads that record. If it encounters a > version it can't support it should probably shutdown since it might not be > able to process any more records. > > However, this does lead to a race condition when a controller is catching > up on the metadata log. Let's say a controller comes up and fetches the > following records from the leader: > > > [M1, M2, M3, FeatureLevelRecord] > > Between the time the controller starts handling requests and it processes > the FeatureLevelRecord, it could report an old value of metadata.version > when handling ApiVersionsRequest. In the brokers, I believe we delay > starting the request handler threads until the broker has been unfenced > (meaning, it is reasonably caught up). We might need something similar for > controllers. > > -David > > On Thu, Oct 14, 2021 at 9:29 PM David Arthur <mum...@gmail.com> wrote: > >> Kowshik, thanks for the review! >> >> 7001. An enum sounds like a good idea here. Especially since setting >> Upgrade=false and Force=true doesn't really make sense. An enum would avoid >> confusing/invalid combinations of flags >> >> 7002. How about adding --force-downgrade as an alternative to the >> --downgrade argument? So, it would take the same arguments (list of >> feature:version), but use the DOWNGRADE_UNSAFE option in the RPC. >> >> 7003. Yes we will need the advanced CLI since we will need to only modify >> the "metadata.version" FF. I was kind of wondering if we might want >> separate sub-commands for the different operations instead of all under >> "update". E.g., "kafka-features.sh >> upgrade|downgrade|force-downgrade|delete|describe". >> >> 7004/7005. The intention in this KIP is that we bump the >> "metadata.version" liberally and that most upgrades are backwards >> compatible. We're relying on this for feature gating as well as indicating >> compatibility. The enum is indeed an implementation detail that is enforced >> by the controller when handling UpdateFeaturesRequest. As for lossy >> downgrades, this really only applies to the metadata log as we will lose >> some fields and records when downgrading to an older version. This is >> useful as an escape hatch for cases when a software upgrade has occurred, >> the feature flag was increased, and then bugs are discovered. Without the >> lossy downgrade scenario, we have no path back to a previous software >> version. >> >> As for the min/max finalized version, I'm not totally clear on cases >> where these would differ. I think for "metadata.version" we just want a >> single finalized version for the whole cluster (like we do for IBP). >> >> -David >> >> >> On Thu, Oct 14, 2021 at 1:59 PM José Armando García Sancio >> <jsan...@confluent.io.invalid> wrote: >> >>> On Tue, Oct 12, 2021 at 10:57 AM Colin McCabe <cmcc...@apache.org> >>> wrote: >>> > > 11. For downgrades, it would be useful to describe how to determine >>> the >>> > > downgrade process (generating new snapshot, propagating the >>> snapshot, etc) >>> > > has completed. We could block the UpdateFeature request until the >>> process >>> > > is completed. However, since the process could take time, the >>> request could >>> > > time out. Another way is through DescribeFeature and the server only >>> > > reports downgraded versions after the process is completed. >>> > >>> > Hmm.. I think we need to avoid blocking, since we don't know how long >>> it will take for all nodes to act on the downgrade request. After all, some >>> nodes may be down. >>> > >>> > But I agree we should have some way of knowing when the upgrade is >>> done. DescribeClusterResponse seems like the natural place to put >>> information about each node's feature level. While we're at it, we should >>> also add a boolean to indicate whether the given node is fenced. (This will >>> always be false for ZK mode, of course...) >>> > >>> >>> I agree. I think from the user's point of view, they would like to >>> know if it is safe to downgrade the software version of a specific >>> broker or controller. I think that it is safe to downgrade a broker or >>> controller when the metadata.version has been downgraded and the node >>> has generated a snapshot with the downgraded metadata.version. >>> >>> -- >>> -Jose >>> >> >> >> -- >> David Arthur >> > > > -- > David Arthur >
