Hi Guozhang, Thanks for the insightful feedback and questions! I have updated the KIP in response to some of the suggestions. Please find my response below.
> 1. Could you explain a bit what would the "the set of features supported by > a broker" information, beyond the cluster-level finalized features, be used > by the client? I think that if we consider all of the features should be > "cluster-wide", i.e. the client may need to talk any brokers of the cluster > to execute specific features, then knowing the supported versions of that > feature from a single broker would not help much, and hence it is > unnecessary to include this information --- maybe I overlooked some use > cases here :) in that case, could you augment the KIP to add some > motivational examples? (Kowshik): This is a good question. You are right, we don't have a use case for a client to make meaningful use of broker-level supported features. In the future this may be useful, but today it is purely part of the API for debuggability reasons. Sometimes, a human may want to look at supported features, and see how it compares against finalized features. I added the supported features to the ApiVersionsResponse, after earlier feedback from Colin hinting about the usefulness of it. Feel free to let us know if you think this is overkill, or confusing, or not required (for now). We could discuss removing the extra field from this KIP. > 2. For the cluster-wide FinalizedFeature (for both ZK storage schema, and > the ApiVersionResponse protocol schema), I understand that our assumption > for using a single version value is that the cluster supports all versions > up to that max-version. However, I wonder if that assumption is appropriate > under normal operations besides Boyang's questions regarding deprecation: > each brokers supported versions for a feature may not always start with > version 0 actually, while a client getting the version X assumes that any > version under X would be supported; so if client only knows about how to > execute a feature on version Y (< X) and then sends a request to broker, > what happens if it only knows at that time that broker actually only > supported min-version larger than Y? Today on the request-level, we either > auto-downgrade the request version (which is not recommended) or we throw > an exception, which could be too late since the client already executed > some steps of that feature which have side-effects persisted. (Kowshik): This is a good question. So far, within the scope of this KIP, we only track the highest level of the finalized feature version (let's call it H). So far, we haven't come across a concrete use case where the client needs to learn the lowest cluster-wide version level (L) in order to make safe + correct decisions at its end. Do you have any use-case in mind? It's open for discussion whether we need to support this in this KIP, or a follow up KIP. Now, below are purely my thoughts, on why we may need this support only rarely (or never). Fundamentally, feature version deprecation is quite a different process than upgrades. During feature version upgrades, the client "may" at some point upgrade to using a newer feature version (at their discretion). On the other hand, feature version deprecation would involve clear cautious steps on the side of the feature provider (i.e. Kafka cluster): 1. Announce the intent to deprecate support for the lowest feature version level L. 2. Check collected data to know that almost all clients have stopped using the feature at version L. 3. Permanently stop accepting requests on the broker corresponding to the feature version L. 4. By this point, we know for sure that no client could be using the feature, because the cluster has stopped taking 100% requests. It is rare for clients to go back to a mode where they will need to decide programmatically, on whether to start using the feature at the deprecated lowest version level L. 5. Now completely drop support for feature version L (ex: eliminate the code in the broker side). This will mean that subsequently brokers will advertise their min_version for the feature at a value V > L and V <= H. As you can see, by the time we hit step 5, it looks unlikely that a client can go back to using the deprecated feature version, unless there is a very drastic change. > 3. Regarding the "Incompatible broker lifetime race condition" section, my > understanding is actually a little bit different, please correct me if I'm > wrong: during broker starts up, after it has registered in ZK along with > its supported versions, the validation procedure is actually executed at > both the broker side as well as the controller: > 3.a) the broker reads the cluster-level feature vectors from ZK directly > and compare with its own supported versions; if the validation fails, it > will shutdown itself, otherwise, proceed normally. > 3.b) upon being notified through ZK watchers of the newly registered > broker, the controller will ALSO execute the validation comparing its > registry's supported feature versions with the cluster-level feature > vectors; if the validation fails, the controller will stop the remaining of > the new-broker-startup procedure like potentially adding it to some > partition's replica list or moving leaders to it. > The key here is that 3.b) on the controller side is serially executed with > all other controller operations, including the add/update/delete-feature > request handling. So if the broker-startup registry is executed first, then > the later update-feature request which would make the broker incompatible > would be rejected; if the update-feature request is handled first, then the > broker-startup logic would abort since the validation fails. In that sense, > there would be no race condition windows -- of course that's based on my > understanding that validation is also executed on the controller side. > Please let me know if that makes sense? (Kowshik): Yes, you are absolutely right. Great point! We are on the same page here. I have updated the KIP. The race condition can be avoided, when in the controller, the thread that handles add/update/delete-feature request is also the same thread that updates the controller's cache of Broker info whenever new brokers join the cluster. In this setup, if an update feature request (E1) is processed ahead of a notification from ZK about an incompatible broker joining the cluster (E2), then the controller can certainly detect that the broker is incompatible when E2 is processed. The controller could stop the remaining of the new broker startup sequence by perhaps refusing to send an UpdateMetadataRequest to bootstrap the new broker. I believe the above is the case today (i.e. controller event processing appears single threaded to me), and I just looked at the code to verify the same. I have made a note of the same on the KIP. Pointers to the code I looked at are below: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/controller/ControllerEventManager.scala#L115 https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/controller/ControllerContext.scala#L78 https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/controller/KafkaController.scala#L1835 Cheers, Kowshik On Sun, Apr 5, 2020 at 10:06 AM Guozhang Wang <wangg...@gmail.com> wrote: > Hello Kowshik, > > Thanks for the great write-up, overall it reads great to me already. Adding > a few meta comments here: > > 1. Could you explain a bit what would the "the set of features supported by > a broker" information, beyond the cluster-level finalized features, be used > by the client? I think that if we consider all of the features should be > "cluster-wide", i.e. the client may need to talk any brokers of the cluster > to execute specific features, then knowing the supported versions of that > feature from a single broker would not help much, and hence it is > unnecessary to include this information --- maybe I overlooked some use > cases here :) in that case, could you augment the KIP to add some > motivational examples? > > 2. For the cluster-wide FinalizedFeature (for both ZK storage schema, and > the ApiVersionResponse protocol schema), I understand that our assumption > for using a single version value is that the cluster supports all versions > up to that max-version. However, I wonder if that assumption is appropriate > under normal operations besides Boyang's questions regarding deprecation: > each brokers supported versions for a feature may not always start with > version 0 actually, while a client getting the version X assumes that any > version under X would be supported; so if client only knows about how to > execute a feature on version Y (< X) and then sends a request to broker, > what happens if it only knows at that time that broker actually only > supported min-version larger than Y? Today on the request-level, we either > auto-downgrade the request version (which is not recommended) or we throw > an exception, which could be too late since the client already executed > some steps of that feature which have side-effects persisted. > > Or do we require clients seeing cluster-wide supporter version X must be > able to execute the feature at exactly version X? If so, that seems too > restrictive to me too. > > 3. Regarding the "Incompatible broker lifetime race condition" section, my > understanding is actually a little bit different, please correct me if I'm > wrong: during broker starts up, after it has registered in ZK along with > its supported versions, the validation procedure is actually executed at > both the broker side as well as the controller: > > 3.a) the broker reads the cluster-level feature vectors from ZK directly > and compare with its own supported versions; if the validation fails, it > will shutdown itself, otherwise, proceed normally. > 3.b) upon being notified through ZK watchers of the newly registered > broker, the controller will ALSO execute the validation comparing its > registry's supported feature versions with the cluster-level feature > vectors; if the validation fails, the controller will stop the remaining of > the new-broker-startup procedure like potentially adding it to some > partition's replica list or moving leaders to it. > > The key here is that 3.b) on the controller side is serially executed with > all other controller operations, including the add/update/delete-feature > request handling. So if the broker-startup registry is executed first, then > the later update-feature request which would make the broker incompatible > would be rejected; if the update-feature request is handled first, then the > broker-startup logic would abort since the validation fails. In that sense, > there would be no race condition windows -- of course that's based on my > understanding that validation is also executed on the controller side. > Please let me know if that makes sense? > > > Guozhang > > > On Sat, Apr 4, 2020 at 8:36 PM Colin McCabe <cmcc...@apache.org> wrote: > > > On Fri, Apr 3, 2020, at 11:24, Jun Rao wrote: > > > Hi, Kowshik, > > > > > > Thanks for the reply. A few more comments below. > > > > > > 100.6 For every new request, the admin needs to control who is allowed > > > to issue that request if security is enabled. So, we need to assign the > > new > > > request a ResourceType and possible AclOperations. See > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment > > > as an example. > > > > > > > Yeah, agreed. To be more specific, the permissions required for this > > should be Alter on Cluster, right? It's certainly something only system > > administrators should be doing (KIP-455 also specifies ALTER on CLUSTER) > > > > best, > > Colin > > > > > > > 105. If we change delete to disable, it's better to do this > consistently > > in > > > request protocol and admin api as well. > > > > > > 110. The minVersion/maxVersion for features use int64. Currently, our > > > release version schema is major.minor.bugfix (e.g. 2.5.0). It's > possible > > > for new features to be included in minor releases too. Should we make > the > > > feature versioning match the release versioning? > > > > > > 111. "During regular operations, the data in the ZK node can be mutated > > > only via a specific admin API served only by the controller." I am > > > wondering why can't the controller auto finalize a feature version > after > > > all brokers are upgraded? For new users who download the latest version > > to > > > build a new cluster, it's inconvenient for them to have to manually > > enable > > > each feature. > > > > > > 112. DeleteFeaturesResponse: It seems the apiKey should be 49 instead > of > > 48. > > > > > > Jun > > > > > > > > > On Fri, Apr 3, 2020 at 1:27 AM Kowshik Prakasam < > kpraka...@confluent.io> > > > wrote: > > > > > > > Hey Jun, > > > > > > > > Thanks a lot for the great feedback! Please note that the design > > > > has changed a little bit on the KIP, and we now propagate the > finalized > > > > features metadata only via ZK watches (instead of > UpdateMetadataRequest > > > > from the controller). > > > > > > > > Please find below my response to your questions/feedback, with the > > prefix > > > > "(Kowshik):". > > > > > > > > > 100. UpdateFeaturesRequest/UpdateFeaturesResponse > > > > > 100.1 Since this request waits for responses from brokers, should > we > > add > > > > a > > > > > timeout in the request (like createTopicRequest)? > > > > > > > > (Kowshik): Great point! Done. I have added a timeout field. Note: we > no > > > > longer > > > > wait for responses from brokers, since the design has been changed so > > that > > > > the > > > > features information is propagated via ZK. Nevertheless, it is right > to > > > > have a timeout > > > > for the request. > > > > > > > > > 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. > > > > > > > > (Kowshik): Great point! Yeah, I have modified it to just return an > > error > > > > code and a message. > > > > Previously it was not echoing the "request", rather it was returning > > the > > > > latest set of > > > > cluster-wide finalized features (after applying the updates). But you > > are > > > > right, > > > > the additional info is not required, so I have removed it from the > > response > > > > schema. > > > > > > > > > 100.3 Should we add a separate request to list/describe the > existing > > > > > features? > > > > > > > > (Kowshik): This is already present in the KIP via the > > 'DescribeFeatures' > > > > Admin API, > > > > which, underneath covers uses the ApiVersionsRequest to list/describe > > the > > > > existing features. Please read the 'Tooling support' section. > > > > > > > > > 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 > > > > > > > > (Kowshik): Great point! I have modified the KIP now to have 2 > separate > > > > controller APIs > > > > serving these different purposes: > > > > 1. updateFeatures > > > > 2. deleteFeatures > > > > > > > > > 100.5 In UpdateFeaturesResponse, we have "The monotonically > > increasing > > > > > version of the metadata for finalized features." I am wondering why > > the > > > > > ordering is important? > > > > > > > > (Kowshik): In the latest KIP write-up, it is called epoch (instead of > > > > version), and > > > > it is just the ZK node version. Basically, this is the epoch for the > > > > cluster-wide > > > > finalized feature version metadata. This metadata is served to > clients > > via > > > > the > > > > ApiVersionsResponse (for reads). We propagate updates from the > > '/features' > > > > ZK node > > > > to all brokers, via ZK watches setup by each broker on the > '/features' > > > > node. > > > > > > > > Now here is why the ordering is important: > > > > ZK watches don't propagate at the same time. As a result, the > > > > ApiVersionsResponse > > > > is eventually consistent across brokers. This can introduce cases > > > > where clients see an older lower epoch of the features metadata, > after > > a > > > > more recent > > > > higher epoch was returned at a previous point in time. We expect > > clients > > > > to always employ the rule that the latest received higher epoch of > > metadata > > > > always trumps an older smaller epoch. Those clients that are external > > to > > > > Kafka should strongly consider discovering the latest metadata once > > during > > > > startup from the brokers, and if required refresh the metadata > > periodically > > > > (to get the latest metadata). > > > > > > > > > 100.6 Could you specify the required ACL for this new request? > > > > > > > > (Kowshik): What is ACL, and how could I find out which one to > specify? > > > > Please could you provide me some pointers? I'll be glad to update the > > > > KIP once I know the next steps. > > > > > > > > > 101. For the broker registration ZK node, should we bump up the > > version > > > > in > > > > the json? > > > > > > > > (Kowshik): Great point! Done. I've increased the version in the > broker > > json > > > > by 1. > > > > > > > > > 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. > > > > > > > > (Kowshik): Great point! Done. I'm using the ZK node version now, > > instead of > > > > explicitly > > > > incremented epoch. > > > > > > > > > 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? > > > > > > > > (Kowshik): Not really. The text was just conveying that a broker > could > > > > "know" of > > > > a new feature version, but it does not mean the broker should have > also > > > > activated the effects of the feature version. Knowing vs activation > > are 2 > > > > separate things, > > > > and the latter can be achieved by dynamic config. I have reworded the > > text > > > > to > > > > make this clear to the reader. > > > > > > > > > > > > > 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."} > > > > > ] > > > > > > > > (Kowshik): With the new improved design, we have completely > eliminated > > the > > > > need to > > > > use UpdateMetadataRequest. This is because we now rely on ZK to > > deliver the > > > > notifications for changes to the '/features' ZK node. > > > > > > > > > 105. kafka-features.sh: Instead of using update/delete, perhaps > it's > > > > better > > > > > to use enable/disable? > > > > > > > > (Kowshik): For delete, yes, I have changed it so that we instead call > > it > > > > 'disable'. > > > > However for 'update', it can now also refer to either an upgrade or a > > > > forced downgrade. > > > > Therefore, I have left it the way it is, just calling it as just > > 'update'. > > > > > > > > > > > > Cheers, > > > > Kowshik > > > > > > > > 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. > > > > > > > > > > > > > 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 > > > > > > . > > > > > > > > > > > > > 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`. > > > > > > > > > > > > > 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. > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > -- Guozhang >
