Hey Boyang, Great point! You are right, thanks for the suggestion! Yes, we can just use ZK watches to propagate finalized features information. I have updated the KIP write up with this change. As a result, I feel the design is simpler as we have also eliminated the changes to UpdateMetadataRequest.
You are right, after exploring/discussing KIP-500 further, we have now realized that taking a ZK dependency here in this KIP just for reads is OK. The future migration path off ZK (in post ZK world) will simply involve reading the finalized features from the controller quorum via the new MetadataFetch API that's proposed in KIP-500. Also note that in the latest KIP write-up, the features metadata epoch is just the ZK node version (as suggested by Jun). Hey Colin, Please feel free to let us know if you have any questions or concerns on the above. Cheers, Kowshik On Thu, Apr 2, 2020 at 10:39 AM Boyang Chen <reluctanthero...@gmail.com> wrote: > Thanks for the reply. The only remaining question is the propagation path. > KIP-500 <https://issues.apache.org/jira/browse/KIP-500> only restricts > `write access` to the controller, in a sense that > brokers in the pre-KIP-500 <https://issues.apache.org/jira/browse/KIP-500> > world could still listen to Zookeeper > notifications. Thus, we are open to discuss the engineering effort to go > through Zookeeper vs UpdateMetadata routing. What's your opinion on this > matter? Will either path significantly simpler than another? > > Boyang > > On Wed, Apr 1, 2020 at 12:10 AM Kowshik Prakasam <kpraka...@confluent.io> > wrote: > > > Hey Boyang, > > > > Thanks for the feedback! Please find below my response to your latest > > comments. > > I have modified the KIP wherever possible to address the comments. > > > > > My point is that during a bootstrapping stage of a cluster, we could > not > > > pick the desired feature version as no controller is actively handling > > our > > > request. > > > > (Kowshik): Note that just deploying the latest broker binary does not > > always mean that the > > new version of a certain feature will be automatically activated. > Enabling > > the effects of the > > actual feature version is still left to the discretion of the > > implementation logic for the feature. > > For example, for safety reasons, the feature can still be gated behind a > > dynamic config > > and later activated when the time comes. > > > > > Feature changes should be roughly the same frequency as config changes. > > > Today, the dynamic configuration changes are propagated via Zookeeper. > > > So I guess propagating through UpdateMetadata doesn't get us more > > benefits, > > > while going through ZK notification should be a simpler solution. > > > > (Kowshik): Maybe I'm missing something, but were you suggesting we should > > have these > > notifications delivered to the brokers directly via ZK? Note that with > > KIP-500 <https://issues.apache.org/jira/browse/KIP-500> (where we are > replacing ZK), > > for the bridge release we prefer that we will perform all access to ZK in > > the controller, > > rather than in other brokers, clients, or tools. Therefore, although ZK > > will still be > > required for the bridge release, it will be a well-isolated dependency. > > Please read > > this section of KIP-500 <https://issues.apache.org/jira/browse/KIP-500>: > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-500%3A+Replace+ZooKeeper+with+a+Self-Managed+Metadata+Quorum#KIP-500:ReplaceZooKeeperwithaSelf-ManagedMetadataQuorum-BridgeRelease > > . > > > > Therefore, the existing approach in the KIP is future proof with regards > to > > the above requirement. > > We deliver the ZK notification only via the controller's > > `UpdateMetadataRequest` to the brokers. > > We also access ZK only always via the controller. > > > > > Understood, I don't feel strong about deprecation, but does the current > > KIP > > > keep the door open for future improvements if > > > someone has a need for feature deprecation? Could we briefly discuss > > about > > > it in the future work section? > > > > (Kowshik): Done. Please refer to the 'Future work' section: > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Futurework > > > > > > Cheers, > > Kowshik > > > > > > On Tue, Mar 31, 2020 at 9:12 PM Boyang Chen <reluctanthero...@gmail.com> > > wrote: > > > > > Thanks Kowshik, the answers are making sense. Some follow-ups: > > > > > > On Tue, Mar 31, 2020 at 6:51 PM Jun Rao <j...@confluent.io> wrote: > > > > > > > Hi, Kowshik, > > > > > > > > Thanks for the KIP. Looks good overall. A few comments below. > > > > > > > > 100. UpdateFeaturesRequest/UpdateFeaturesResponse > > > > 100.1 Since this request waits for responses from brokers, should we > > add > > > a > > > > timeout in the request (like createTopicRequest)? > > > > 100.2 The response schema is a bit weird. Typically, the response > just > > > > shows an error code and an error message, instead of echoing the > > request. > > > > 100.3 Should we add a separate request to list/describe the existing > > > > features? > > > > 100.4 We are mixing ADD_OR_UPDATE and DELETE in a single request. For > > > > DELETE, the version field doesn't make sense. So, I guess the broker > > just > > > > ignores this? An alternative way is to have a separate > > > > DeleteFeaturesRequest > > > > 100.5 In UpdateFeaturesResponse, we have "The monotonically > increasing > > > > version of the metadata for finalized features." I am wondering why > the > > > > ordering is important? > > > > 100.6 Could you specify the required ACL for this new request? > > > > > > > > 101. For the broker registration ZK node, should we bump up the > version > > > in > > > > the json? > > > > > > > > 102. For the /features ZK node, not sure if we need the epoch field. > > Each > > > > ZK node has an internal version field that is incremented on every > > > update. > > > > > > > > 103. "Enabling the actual semantics of a feature version cluster-wide > > is > > > > left to the discretion of the logic implementing the feature (ex: can > > be > > > > done via dynamic broker config)." Does that mean the broker > > registration > > > ZK > > > > node will be updated dynamically when this happens? > > > > > > > > 104. UpdateMetadataRequest > > > > 104.1 It would be useful to describe when the feature metadata is > > > included > > > > in the request. My understanding is that it's only included if (1) > > there > > > is > > > > a change to the finalized feature; (2) broker restart; (3) controller > > > > failover. > > > > 104.2 The new fields have the following versions. Why are the > versions > > 3+ > > > > when the top version is bumped to 6? > > > > "fields": [ > > > > {"name": "Name", "type": "string", "versions": "3+", > > > > "about": "The name of the feature."}, > > > > {"name": "Version", "type": "int64", "versions": "3+", > > > > "about": "The finalized version for the feature."} > > > > ] > > > > > > > > 105. kafka-features.sh: Instead of using update/delete, perhaps it's > > > better > > > > to use enable/disable? > > > > > > > > Jun > > > > > > > > On Tue, Mar 31, 2020 at 5:29 PM Kowshik Prakasam < > > kpraka...@confluent.io > > > > > > > > wrote: > > > > > > > > > Hey Boyang, > > > > > > > > > > Thanks for the great feedback! I have updated the KIP based on your > > > > > feedback. > > > > > Please find my response below for your comments, look for sentences > > > > > starting > > > > > with "(Kowshik)" below. > > > > > > > > > > > > > > > > 1. "When is it safe for the brokers to begin handling EOS > traffic" > > > > could > > > > > be > > > > > > converted as "When is it safe for the brokers to start serving > new > > > > > > Exactly-Once(EOS) semantics" since EOS is not explained earlier > in > > > the > > > > > > context. > > > > > > > > > > (Kowshik): Great point! Done. > > > > > > > > > > > > > 2. In the *Explanation *section, the metadata version number part > > seems > > > > a > > > > > > bit blurred. Could you point a reference to later section that we > > > going > > > > > to > > > > > > store it in Zookeeper and update it every time when there is a > > > feature > > > > > > change? > > > > > > > > > > (Kowshik): Great point! Done. I've added a reference in the KIP. > > > > > > > > > > > > > > > > 3. For the feature downgrade, although it's a Non-goal of the > KIP, > > > for > > > > > > features such as group coordinator semantics, there is no legal > > > > scenario > > > > > to > > > > > > perform a downgrade at all. So having downgrade door open is > pretty > > > > > > error-prone as human faults happen all the time. I'm assuming as > > new > > > > > > features are implemented, it's not very hard to add a flag during > > > > feature > > > > > > creation to indicate whether this feature is "downgradable". > Could > > > you > > > > > > explain a bit more on the extra engineering effort for shipping > > this > > > > KIP > > > > > > with downgrade protection in place? > > > > > > > > > > (Kowshik): Great point! I'd agree and disagree here. While I agree > > that > > > > > accidental > > > > > downgrades can cause problems, I also think sometimes downgrades > > should > > > > > be allowed for emergency reasons (not all downgrades cause issues). > > > > > It is just subjective to the feature being downgraded. > > > > > > > > > > To be more strict about feature version downgrades, I have modified > > the > > > > KIP > > > > > proposing that we mandate a `--force-downgrade` flag be used in the > > > > > UPDATE_FEATURES api > > > > > and the tooling, whenever the human is downgrading a finalized > > feature > > > > > version. > > > > > Hopefully this should cover the requirement, until we find the need > > for > > > > > advanced downgrade support. > > > > > > > > > > > > +1 for adding this flag. > > > > > > > > > 4. "Each broker’s supported dictionary of feature versions will > be > > > > > defined > > > > > > in the broker code." So this means in order to restrict a certain > > > > > feature, > > > > > > we need to start the broker first and then send a feature gating > > > > request > > > > > > immediately, which introduces a time gap and the > intended-to-close > > > > > feature > > > > > > could actually serve request during this phase. Do you think we > > > should > > > > > also > > > > > > support configurations as well so that admin user could freely > roll > > > up > > > > a > > > > > > cluster with all nodes complying the same feature gating, without > > > > > worrying > > > > > > about the turnaround time to propagate the message only after the > > > > cluster > > > > > > starts up? > > > > > > > > > > (Kowshik): This is a great point/question. One of the expectations > > out > > > of > > > > > this KIP, which is > > > > > already followed in the broker, is the following. > > > > > - Imagine at time T1 the broker starts up and registers it’s > > presence > > > in > > > > > ZK, > > > > > along with advertising it’s supported features. > > > > > - Imagine at a future time T2 the broker receives the > > > > > UpdateMetadataRequest > > > > > from the controller, which contains the latest finalized > features > > as > > > > > seen by > > > > > the controller. The broker validates this data against it’s > > > supported > > > > > features to > > > > > make sure there is no mismatch (it will shutdown if there is an > > > > > incompatibility). > > > > > > > > > > It is expected that during the time between the 2 events T1 and T2, > > the > > > > > broker is > > > > > almost a silent entity in the cluster. It does not add any value to > > the > > > > > cluster, or carry > > > > > out any important broker activities. By “important”, I mean it is > not > > > > doing > > > > > mutations > > > > > on it’s persistence, not mutating critical in-memory state, won’t > be > > > > > serving > > > > > produce/fetch requests. Note it doesn’t even know it’s assigned > > > > partitions > > > > > until > > > > > it receives UpdateMetadataRequest from controller. Anything the > > broker > > > is > > > > > doing up > > > > > until this point is not damaging/useful. > > > > > > > > > > I’ve clarified the above in the KIP, see this new section: > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Incompatiblebrokerlifetime > > > > > . > > > > > > > > My point is that during a bootstrapping stage of a cluster, we could > > not > > > pick the desired feature version as no controller is actively handling > > our > > > request. But anyway, I think this is a rare case to discuss, and the > > added > > > paragraph looks good :) > > > > > > > > > > > > 5. "adding a new Feature, updating or deleting an existing > > Feature", > > > > may > > > > > be > > > > > > I misunderstood something, I thought the features are defined in > > > broker > > > > > > code, so admin could not really create a new feature? > > > > > > > > > > (Kowshik): Great point! You understood this right. Here adding a > > > feature > > > > > means we are > > > > > adding a cluster-wide finalized *max* version for a feature that > was > > > > > previously never finalized. > > > > > I have clarified this in the KIP now. > > > > > > > > > > > 6. I think we need a separate error code like > > > > FEATURE_UPDATE_IN_PROGRESS > > > > > to > > > > > > reject a concurrent feature update request. > > > > > > > > > > (Kowshik): Great point! I have modified the KIP adding the above > (see > > > > > 'Tooling support -> Admin API changes'). > > > > > > > > > > > 7. I think we haven't discussed the alternative solution to pass > > the > > > > > > feature information through Zookeeper. Is that mentioned in the > KIP > > > to > > > > > > justify why using UpdateMetadata is more favorable? > > > > > > > > > > (Kowshik): Nice question! The broker reads finalized feature info > > > stored > > > > in > > > > > ZK, > > > > > only during startup when it does a validation. When serving > > > > > `ApiVersionsRequest`, the > > > > > broker does not read this info from ZK directly. I'd imagine the > risk > > > is > > > > > that it can increase > > > > > the ZK read QPS which can be a bottleneck for the system. Today, in > > > Kafka > > > > > we use the > > > > > controller to fan out ZK updates to brokers and we want to stick to > > > that > > > > > pattern to avoid > > > > > the ZK read bottleneck when serving `ApiVersionsRequest`. > > > > > > > > Feature changes should be roughly the same frequency as config > changes. > > > Today, the dynamic configuration > > > changes are propagated via Zookeeper. So I guess propagating through > > > UpdateMetadata doesn't get us more benefits, > > > while going through ZK notification should be a simpler solution. > > > > > > > > 8. I was under the impression that user could configure a range of > > > > > > supported versions, what's the trade-off for allowing single > > > finalized > > > > > > version only? > > > > > > > > > > (Kowshik): Great question! The finalized version of a feature > > basically > > > > > refers to > > > > > the cluster-wide finalized feature "maximum" version. For example, > if > > > the > > > > > 'group_coordinator' feature > > > > > has the finalized version set to 10, then, it means that > cluster-wide > > > all > > > > > versions upto v10 are > > > > > supported for this feature. However, note that if some version (ex: > > v0) > > > > > gets deprecated > > > > > for this feature, then we don’t convey that using this scheme (also > > > > > supporting deprecation is a non-goal). > > > > > > > > > > (Kowshik): I’ve now modified the KIP at all points, refering to > > > finalized > > > > > feature "maximum" versions. > > > > > > > > > > > > Understood, I don't feel strong about deprecation, but does the current > > KIP > > > keep the door open for future improvements if > > > someone has a need for feature deprecation? Could we briefly discuss > > about > > > it in the future work section? > > > > > > > > > > > > 9. One minor syntax fix: Note that here the "client" here may be > a > > > > > producer > > > > > > > > > > (Kowshik): Great point! Done. > > > > > > > > > > > > > > > Cheers, > > > > > Kowshik > > > > > > > > > > > > > > > On Tue, Mar 31, 2020 at 1:17 PM Boyang Chen < > > > reluctanthero...@gmail.com> > > > > > wrote: > > > > > > > > > > > Hey Kowshik, > > > > > > > > > > > > thanks for the revised KIP. Got a couple of questions: > > > > > > > > > > > > 1. "When is it safe for the brokers to begin handling EOS > traffic" > > > > could > > > > > be > > > > > > converted as "When is it safe for the brokers to start serving > new > > > > > > Exactly-Once(EOS) semantics" since EOS is not explained earlier > in > > > the > > > > > > context. > > > > > > > > > > > > 2. In the *Explanation *section, the metadata version number part > > > > seems a > > > > > > bit blurred. Could you point a reference to later section that we > > > going > > > > > to > > > > > > store it in Zookeeper and update it every time when there is a > > > feature > > > > > > change? > > > > > > > > > > > > 3. For the feature downgrade, although it's a Non-goal of the > KIP, > > > for > > > > > > features such as group coordinator semantics, there is no legal > > > > scenario > > > > > to > > > > > > perform a downgrade at all. So having downgrade door open is > pretty > > > > > > error-prone as human faults happen all the time. I'm assuming as > > new > > > > > > features are implemented, it's not very hard to add a flag during > > > > feature > > > > > > creation to indicate whether this feature is "downgradable". > Could > > > you > > > > > > explain a bit more on the extra engineering effort for shipping > > this > > > > KIP > > > > > > with downgrade protection in place? > > > > > > > > > > > > 4. "Each broker’s supported dictionary of feature versions will > be > > > > > defined > > > > > > in the broker code." So this means in order to restrict a certain > > > > > feature, > > > > > > we need to start the broker first and then send a feature gating > > > > request > > > > > > immediately, which introduces a time gap and the > intended-to-close > > > > > feature > > > > > > could actually serve request during this phase. Do you think we > > > should > > > > > also > > > > > > support configurations as well so that admin user could freely > roll > > > up > > > > a > > > > > > cluster with all nodes complying the same feature gating, without > > > > > worrying > > > > > > about the turnaround time to propagate the message only after the > > > > cluster > > > > > > starts up? > > > > > > > > > > > > 5. "adding a new Feature, updating or deleting an existing > > Feature", > > > > may > > > > > be > > > > > > I misunderstood something, I thought the features are defined in > > > broker > > > > > > code, so admin could not really create a new feature? > > > > > > > > > > > > 6. I think we need a separate error code like > > > > FEATURE_UPDATE_IN_PROGRESS > > > > > to > > > > > > reject a concurrent feature update request. > > > > > > > > > > > > 7. I think we haven't discussed the alternative solution to pass > > the > > > > > > feature information through Zookeeper. Is that mentioned in the > KIP > > > to > > > > > > justify why using UpdateMetadata is more favorable? > > > > > > > > > > > > 8. I was under the impression that user could configure a range > of > > > > > > supported versions, what's the trade-off for allowing single > > > finalized > > > > > > version only? > > > > > > > > > > > > 9. One minor syntax fix: Note that here the "client" here may be > a > > > > > producer > > > > > > > > > > > > Boyang > > > > > > > > > > > > On Mon, Mar 30, 2020 at 4:53 PM Colin McCabe <cmcc...@apache.org > > > > > > wrote: > > > > > > > > > > > > > On Thu, Mar 26, 2020, at 19:24, Kowshik Prakasam wrote: > > > > > > > > Hi Colin, > > > > > > > > > > > > > > > > Thanks for the feedback! I've changed the KIP to address your > > > > > > > > suggestions. > > > > > > > > Please find below my explanation. Here is a link to KIP 584: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features > > > > > > > > . > > > > > > > > > > > > > > > > 1. '__data_version__' is the version of the finalized feature > > > > > metadata > > > > > > > > (i.e. actual ZK node contents), while the > '__schema_version__' > > is > > > > the > > > > > > > > version of the schema of the data persisted in ZK. These > serve > > > > > > different > > > > > > > > purposes. '__data_version__' is is useful mainly to clients > > > during > > > > > > reads, > > > > > > > > to differentiate between the 2 versions of eventually > > consistent > > > > > > > 'finalized > > > > > > > > features' metadata (i.e. larger metadata version is more > > recent). > > > > > > > > '__schema_version__' provides an additional degree of > > > flexibility, > > > > > > where > > > > > > > if > > > > > > > > we decide to change the schema for '/features' node in ZK (in > > the > > > > > > > future), > > > > > > > > then we can manage broker roll outs suitably (i.e. > > > > > > > > serialization/deserialization of the ZK data can be handled > > > > safely). > > > > > > > > > > > > > > Hi Kowshik, > > > > > > > > > > > > > > If you're talking about a number that lets you know if data is > > more > > > > or > > > > > > > less recent, we would typically call that an epoch, and not a > > > > version. > > > > > > For > > > > > > > the ZK data structures, the word "version" is typically > reserved > > > for > > > > > > > describing changes to the overall schema of the data that is > > > written > > > > to > > > > > > > ZooKeeper. We don't even really change the "version" of those > > > > schemas > > > > > > that > > > > > > > much, since most changes are backwards-compatible. But we do > > > include > > > > > > that > > > > > > > version field just in case. > > > > > > > > > > > > > > I don't think we really need an epoch here, though, since we > can > > > just > > > > > > look > > > > > > > at the broker epoch. Whenever the broker registers, its epoch > > will > > > > be > > > > > > > greater than the previous broker epoch. And the newly > registered > > > > data > > > > > > will > > > > > > > take priority. This will be a lot simpler than adding a > separate > > > > epoch > > > > > > > system, I think. > > > > > > > > > > > > > > > > > > > > > > > 2. Regarding admin client needing min and max information - > you > > > are > > > > > > > right! > > > > > > > > I've changed the KIP such that the Admin API also allows the > > user > > > > to > > > > > > read > > > > > > > > 'supported features' from a specific broker. Please look at > the > > > > > section > > > > > > > > "Admin API changes". > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > 3. Regarding the use of `long` vs `Long` - it was not > > deliberate. > > > > > I've > > > > > > > > improved the KIP to just use `long` at all places. > > > > > > > > > > > > > > Sounds good. > > > > > > > > > > > > > > > > > > > > > > > 4. Regarding kafka.admin.FeatureCommand tool - you are right! > > > I've > > > > > > > updated > > > > > > > > the KIP sketching the functionality provided by this tool, > with > > > > some > > > > > > > > examples. Please look at the section "Tooling support > > examples". > > > > > > > > > > > > > > > > Thank you! > > > > > > > > > > > > > > > > > > > > > Thanks, Kowshik. > > > > > > > > > > > > > > cheers, > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > Kowshik > > > > > > > > > > > > > > > > On Wed, Mar 25, 2020 at 11:31 PM Colin McCabe < > > > cmcc...@apache.org> > > > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks, Kowshik, this looks good. > > > > > > > > > > > > > > > > > > In the "Schema" section, do we really need both > > > > __schema_version__ > > > > > > and > > > > > > > > > __data_version__? Can we just have a single version field > > > here? > > > > > > > > > > > > > > > > > > Shouldn't the Admin(Client) function have some way to get > the > > > min > > > > > and > > > > > > > max > > > > > > > > > information that we're exposing as well? I guess we could > > have > > > > > min, > > > > > > > max, > > > > > > > > > and current. Unrelated: is the use of Long rather than > long > > > > > > deliberate > > > > > > > > > here? > > > > > > > > > > > > > > > > > > It would be good to describe how the command line tool > > > > > > > > > kafka.admin.FeatureCommand will work. For example the > flags > > > that > > > > > it > > > > > > > will > > > > > > > > > take and the output that it will generate to STDOUT. > > > > > > > > > > > > > > > > > > cheers, > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Mar 24, 2020, at 17:08, Kowshik Prakasam wrote: > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > I've opened KIP-584 > <https://issues.apache.org/jira/browse/KIP-584> < > > > > > https://issues.apache.org/jira/browse/KIP-584 > > > > > > > > > > > > > > > > > which > > > > > > > > > > is intended to provide a versioning scheme for features. > > I'd > > > > like > > > > > > to > > > > > > > use > > > > > > > > > > this thread to discuss the same. I'd appreciate any > > feedback > > > on > > > > > > this. > > > > > > > > > > Here > > > > > > > > > > is a link to KIP-584 > <https://issues.apache.org/jira/browse/KIP-584>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features > > > > > > > > > > . > > > > > > > > > > > > > > > > > > > > Thank you! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > Kowshik > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
