Re: [DISCUSS] KIP-584: Versioning scheme for features

Kowshik Prakasam Wed, 15 Apr 2020 13:09:08 -0700

Hi Jun,

Thank you for the suggestion! I have updated the KIP, please find my
response below.


> 200. I guess you are saying only when the allowDowngrade field is set, the
> finalized feature version can go backward. Otherwise, it can only go up.
> That makes sense. It would be useful to make that clear when explaining
> the usage of the allowDowngrade field. In the validation section, we
have  "
> /features' from {"max_version_level": X} to {"max_version_level": X’}", it
> seems that we need to mention Y there.

(Kowshik): Great point! Yes, that is correct. Done, I have updated the
validations
section explaining the above. Here is a link to this section:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Validations


Cheers,
Kowshik




On Wed, Apr 15, 2020 at 11:05 AM Jun Rao <j...@confluent.io> wrote:

> Hi, Kowshik,
>
> 200. I guess you are saying only when the allowDowngrade field is set, the
> finalized feature version can go backward. Otherwise, it can only go up.
> That makes sense. It would be useful to make that clear when explaining
> the usage of the allowDowngrade field. In the validation section, we have
> "
> /features' from {"max_version_level": X} to {"max_version_level": X’}", it
> seems that we need to mention Y there.
>
> Thanks,
>
> Jun
>
> On Wed, Apr 15, 2020 at 10:44 AM Kowshik Prakasam <kpraka...@confluent.io>
> wrote:
>
> > Hi Jun,
> >
> > Great question! Please find my response below.
> >
> > > 200. My understanding is that If the CLI tool passes the
> > > '--allow-downgrade' flag when updating a specific feature, then a
> future
> > > downgrade is possible. Otherwise, the feature is now downgradable. If
> so,
> > I
> > > was wondering how the controller remembers this since it can be
> restarted
> > > over time?
> >
> > (Kowshik): The purpose of the flag was to just restrict the user intent
> for
> > a specific request.
> > It seems to me that to avoid confusion, I could call the flag as
> > `--try-downgrade` instead.
> > Then this makes it clear, that, the controller just has to consider the
> ask
> > from
> > the user as an explicit request to attempt a downgrade.
> >
> > The flag does not act as an override on controller's decision making that
> > decides whether
> > a flag is downgradable (these decisions on whether to allow a flag to be
> > downgraded
> > from a specific version level, can be embedded in the controller code).
> >
> > Please let me know what you think.
> > Sorry if I misunderstood the original question.
> >
> >
> > Cheers,
> > Kowshik
> >
> >
> > On Wed, Apr 15, 2020 at 9:40 AM Jun Rao <j...@confluent.io> wrote:
> >
> > > Hi, Kowshik,
> > >
> > > Thanks for the reply. Makes sense. Just one more question.
> > >
> > > 200. My understanding is that If the CLI tool passes the
> > > '--allow-downgrade' flag when updating a specific feature, then a
> future
> > > downgrade is possible. Otherwise, the feature is now downgradable. If
> > so, I
> > > was wondering how the controller remembers this since it can be
> restarted
> > > over time?
> > >
> > > Jun
> > >
> > >
> > > On Tue, Apr 14, 2020 at 6:49 PM Kowshik Prakasam <
> kpraka...@confluent.io
> > >
> > > wrote:
> > >
> > > > Hi Jun,
> > > >
> > > > Thanks a lot for the feedback and the questions!
> > > > Please find my response below.
> > > >
> > > > > 200. The UpdateFeaturesRequest includes an AllowDowngrade field. It
> > > seems
> > > > > that field needs to be persisted somewhere in ZK?
> > > >
> > > > (Kowshik): Great question! Below is my explanation. Please help me
> > > > understand,
> > > > if you feel there are cases where we would need to still persist it
> in
> > > ZK.
> > > >
> > > > Firstly I have updated my thoughts into the KIP now, under the
> > > 'guidelines'
> > > > section:
> > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Guidelinesonfeatureversionsandworkflows
> > > >
> > > > The allowDowngrade boolean field is just to restrict the user intent,
> > and
> > > > to remind
> > > > them to double check their intent before proceeding. It should be set
> > to
> > > > true
> > > > by the user in a request, only when the user intent is to forcefully
> > > > "attempt" a
> > > > downgrade of a specific feature's max version level, to the provided
> > > value
> > > > in
> > > > the request.
> > > >
> > > > We can extend this safeguard. The controller (on it's end) can
> maintain
> > > > rules in the code, that, for safety reasons would outright reject
> > certain
> > > > downgrades
> > > > from a specific max_version_level for a specific feature. Such
> > rejections
> > > > may
> > > > happen depending on the feature being downgraded, and from what
> version
> > > > level.
> > > >
> > > > The CLI tool only allows a downgrade attempt in conjunction with
> > specific
> > > > flags and sub-commands. For example, in the CLI tool, if the user
> uses
> > > the
> > > > 'downgrade-all' command, or passes '--allow-downgrade' flag when
> > > updating a
> > > > specific feature, only then the tool will translate this ask to
> setting
> > > > 'allowDowngrade' field in the request to the server.
> > > >
> > > > > 201. UpdateFeaturesResponse has the following top level fields.
> > Should
> > > > > those fields be per feature?
> > > > >
> > > > >   "fields": [
> > > > >     { "name": "ErrorCode", "type": "int16", "versions": "0+",
> > > > >       "about": "The error code, or 0 if there was no error." },
> > > > >     { "name": "ErrorMessage", "type": "string", "versions": "0+",
> > > > >       "about": "The error message, or null if there was no error."
> }
> > > > >   ]
> > > >
> > > > (Kowshik): Great question!
> > > > As such, the API is transactional, as explained in the sections
> linked
> > > > below.
> > > > Either all provided FeatureUpdate was applied, or none.
> > > > It's the reason I felt we can have just one error code + message.
> > > > Happy to extend this if you feel otherwise. Please let me know.
> > > >
> > > > Link to sections:
> > > >
> > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-ChangestoKafkaController
> > > >
> > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Guarantees
> > > >
> > > > > 202. The /features path in ZK has a field min_version_level. Which
> > API
> > > > and
> > > > > tool can change that value?
> > > >
> > > > (Kowshik): Great question! Currently this cannot be modified by using
> > the
> > > > API or the tool.
> > > > Feature version deprecation (by raising min_version_level) can be
> done
> > > only
> > > > by the Controller directly. The rationale is explained in this
> section:
> > > >
> > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Featureversiondeprecation
> > > >
> > > >
> > > > Cheers,
> > > > Kowshik
> > > >
> > > > On Tue, Apr 14, 2020 at 5:33 PM Jun Rao <j...@confluent.io> wrote:
> > > >
> > > > > Hi, Kowshik,
> > > > >
> > > > > Thanks for addressing those comments. Just a few more minor
> comments.
> > > > >
> > > > > 200. The UpdateFeaturesRequest includes an AllowDowngrade field. It
> > > seems
> > > > > that field needs to be persisted somewhere in ZK?
> > > > >
> > > > > 201. UpdateFeaturesResponse has the following top level fields.
> > Should
> > > > > those fields be per feature?
> > > > >
> > > > >   "fields": [
> > > > >     { "name": "ErrorCode", "type": "int16", "versions": "0+",
> > > > >       "about": "The error code, or 0 if there was no error." },
> > > > >     { "name": "ErrorMessage", "type": "string", "versions": "0+",
> > > > >       "about": "The error message, or null if there was no error."
> }
> > > > >   ]
> > > > >
> > > > > 202. The /features path in ZK has a field min_version_level. Which
> > API
> > > > and
> > > > > tool can change that value?
> > > > >
> > > > > Jun
> > > > >
> > > > > On Mon, Apr 13, 2020 at 5:12 PM Kowshik Prakasam <
> > > kpraka...@confluent.io
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Hi Jun,
> > > > > >
> > > > > > Thanks for the feedback! I have updated the KIP-584 addressing
> your
> > > > > > comments.
> > > > > > Please find my response below.
> > > > > >
> > > > > > > 100.6 You can look for the sentence "This operation requires
> > ALTER
> > > on
> > > > > > > CLUSTER." in KIP-455. Also, you can check its usage in
> > > > > > > KafkaApis.authorize().
> > > > > >
> > > > > > (Kowshik): Done. Great point! For the newly introduced
> > > UPDATE_FEATURES
> > > > > api,
> > > > > > I have added a
> > > > > > requirement that AclOperation.ALTER is required on
> > > > ResourceType.CLUSTER.
> > > > > >
> > > > > > > 110. Keeping the feature version as int is probably fine. I
> just
> > > felt
> > > > > > that
> > > > > > > for some of the common user interactions, it's more convenient
> to
> > > > > > > relate that to a release version. For example, if a user wants
> to
> > > > > > downgrade
> > > > > > > to a release 2.5, it's easier for the user to use the tool like
> > > "tool
> > > > > > > --downgrade 2.5" instead of "tool --downgrade --feature X
> > --version
> > > > 6".
> > > > > >
> > > > > > (Kowshik): Great point. Generally, maximum feature version levels
> > are
> > > > not
> > > > > > downgradable after
> > > > > > they are finalized in the cluster. This is because, as a
> guideline
> > > > > bumping
> > > > > > feature version level usually is used mainly to convey important
> > > > breaking
> > > > > > changes.
> > > > > > Despite the above, there may be some extreme/rare cases where a
> > user
> > > > > wants
> > > > > > to downgrade
> > > > > > all features to a specific previous release. The user may want to
> > do
> > > > this
> > > > > > just
> > > > > > prior to rolling back a Kafka cluster to a previous release.
> > > > > >
> > > > > > To support the above, I have made a change to the KIP explaining
> > that
> > > > the
> > > > > > CLI tool is versioned.
> > > > > > The CLI tool internally has knowledge about a map of features to
> > > their
> > > > > > respective max
> > > > > > versions supported by the Broker. The tool's knowledge of
> features
> > > and
> > > > > > their version values,
> > > > > > is limited to the version of the CLI tool itself i.e. the
> > information
> > > > is
> > > > > > packaged into the CLI tool
> > > > > > when it is released. Whenever a Kafka release introduces a new
> > > feature
> > > > > > version, or modifies
> > > > > > an existing feature version, the CLI tool shall also be updated
> > with
> > > > this
> > > > > > information,
> > > > > > Newer versions of the CLI tool will be released as part of the
> > Kafka
> > > > > > releases.
> > > > > >
> > > > > > Therefore, to achieve the downgrade need, the user just needs to
> > run
> > > > the
> > > > > > version of
> > > > > > the CLI tool that's part of the particular previous release that
> > > he/she
> > > > > is
> > > > > > downgrading to.
> > > > > > To help the user with this, there is a new command added to the
> CLI
> > > > tool
> > > > > > called `downgrade-all`.
> > > > > > This essentially downgrades max version levels of all features in
> > the
> > > > > > cluster to the versions
> > > > > > known to the CLI tool internally.
> > > > > >
> > > > > > I have explained the above in the KIP under these sections:
> > > > > >
> > > > > > Tooling support (have explained that the CLI tool is versioned):
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Toolingsupport
> > > > > >
> > > > > > Regular CLI tool usage (please refer to point #3, and see the
> > tooling
> > > > > > example)
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-RegularCLItoolusage
> > > > > >
> > > > > > > 110. Similarly, if the client library finds a feature mismatch
> > with
> > > > the
> > > > > > broker,
> > > > > > > the client likely needs to log some error message for the user
> to
> > > > take
> > > > > > some
> > > > > > > actions. It's much more actionable if the error message is
> > "upgrade
> > > > the
> > > > > > > broker to release version 2.6" than just "upgrade the broker to
> > > > feature
> > > > > > > version 7".
> > > > > >
> > > > > > (Kowshik): That's a really good point! If we use ints for feature
> > > > > versions,
> > > > > > the best
> > > > > > message that client can print for debugging is "broker doesn't
> > > support
> > > > > > feature version 7", and alongside that print the supported
> version
> > > > range
> > > > > > returned
> > > > > > by the broker. Then, does it sound reasonable that the user could
> > > then
> > > > > > reference
> > > > > > Kafka release logs to figure out which version of the broker
> > release
> > > is
> > > > > > required
> > > > > > be deployed, to support feature version 7? I couldn't think of a
> > > better
> > > > > > strategy here.
> > > > > >
> > > > > > > 120. When should a developer bump up the version of a feature?
> > > > > >
> > > > > > (Kowshik): Great question! In the KIP, I have added a section:
> > > > > 'Guidelines
> > > > > > on feature versions and workflows'
> > > > > > providing some guidelines on when to use the versioned feature
> > flags,
> > > > and
> > > > > > what
> > > > > > are the regular workflows with the CLI tool.
> > > > > >
> > > > > > Link to the relevant sections:
> > > > > > Guidelines:
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Guidelinesonfeatureversionsandworkflows
> > > > > >
> > > > > > Regular CLI tool usage:
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-RegularCLItoolusage
> > > > > >
> > > > > > Advanced CLI tool usage:
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-AdvancedCLItoolusage
> > > > > >
> > > > > >
> > > > > > Cheers,
> > > > > > Kowshik
> > > > > >
> > > > > >
> > > > > > On Fri, Apr 10, 2020 at 4:25 PM Jun Rao <j...@confluent.io>
> wrote:
> > > > > >
> > > > > > > Hi, Kowshik,
> > > > > > >
> > > > > > > Thanks for the reply. A few more comments.
> > > > > > >
> > > > > > > 110. Keeping the feature version as int is probably fine. I
> just
> > > felt
> > > > > > that
> > > > > > > for some of the common user interactions, it's more convenient
> to
> > > > > > > relate that to a release version. For example, if a user wants
> to
> > > > > > downgrade
> > > > > > > to a release 2.5, it's easier for the user to use the tool like
> > > "tool
> > > > > > > --downgrade 2.5" instead of "tool --downgrade --feature X
> > --version
> > > > 6".
> > > > > > > Similarly, if the client library finds a feature mismatch with
> > the
> > > > > > broker,
> > > > > > > the client likely needs to log some error message for the user
> to
> > > > take
> > > > > > some
> > > > > > > actions. It's much more actionable if the error message is
> > "upgrade
> > > > the
> > > > > > > broker to release version 2.6" than just "upgrade the broker to
> > > > feature
> > > > > > > version 7".
> > > > > > >
> > > > > > > 111. Sounds good.
> > > > > > >
> > > > > > > 120. When should a developer bump up the version of a feature?
> > > > > > >
> > > > > > > Jun
> > > > > > >
> > > > > > > On Tue, Apr 7, 2020 at 12:26 AM Kowshik Prakasam <
> > > > > kpraka...@confluent.io
> > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Jun,
> > > > > > > >
> > > > > > > > I have updated the KIP for the item 111.
> > > > > > > > I'm in the process of addressing 100.6, and will provide an
> > > update
> > > > > > soon.
> > > > > > > > I think item 110 is still under discussion given we are now
> > > > > providing a
> > > > > > > way
> > > > > > > > to finalize
> > > > > > > > all features to their latest version levels. In any case,
> > please
> > > > let
> > > > > us
> > > > > > > > know
> > > > > > > > how you feel in response to Colin's comments on this topic.
> > > > > > > >
> > > > > > > > > 111. To put this in context, when we had IBP, the default
> > value
> > > > is
> > > > > > the
> > > > > > > > > current released version. So, if you are a brand new user,
> > you
> > > > > don't
> > > > > > > need
> > > > > > > > > to configure IBP and all new features will be immediately
> > > > available
> > > > > > in
> > > > > > > > the
> > > > > > > > > new cluster. If you are upgrading from an old version, you
> do
> > > > need
> > > > > to
> > > > > > > > > understand and configure IBP. I see a similar pattern here
> > for
> > > > > > > > > features. From the ease of use perspective, ideally, we
> > > shouldn't
> > > > > > > require
> > > > > > > > a
> > > > > > > > > new user to have an extra step such as running a bootstrap
> > > script
> > > > > > > unless
> > > > > > > > > it's truly necessary. If someone has a special need (all
> the
> > > > cases
> > > > > > you
> > > > > > > > > mentioned seem special cases?), they can configure a mode
> > such
> > > > that
> > > > > > > > > features are enabled/disabled manually.
> > > > > > > >
> > > > > > > > (Kowshik): That makes sense, thanks for the idea! Sorry if I
> > > didn't
> > > > > > > > understand
> > > > > > > > this need earlier. I have updated the KIP with the approach
> > that
> > > > > > whenever
> > > > > > > > the '/features' node is absent, the controller by default
> will
> > > > > > bootstrap
> > > > > > > > the node
> > > > > > > > to contain the latest feature levels. Here is the new section
> > in
> > > > the
> > > > > > KIP
> > > > > > > > describing
> > > > > > > > the same:
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Controller:ZKnodebootstrapwithdefaultvalues
> > > > > > > >
> > > > > > > > Next, as I explained in my response to Colin's suggestions,
> we
> > > are
> > > > > now
> > > > > > > > providing a `--finalize-latest-features` flag with the
> tooling.
> > > > This
> > > > > > lets
> > > > > > > > the sysadmin finalize all features known to the controller to
> > > their
> > > > > > > latest
> > > > > > > > version
> > > > > > > > levels. Please look at this section (point #3 and the tooling
> > > > example
> > > > > > > > later):
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-584%3A+Versioning+scheme+for+features#KIP-584:Versioningschemeforfeatures-Toolingsupport
> > > > > > > >
> > > > > > > >
> > > > > > > > Do you feel this addresses your comment/concern?
> > > > > > > >
> > > > > > > >
> > > > > > > > Cheers,
> > > > > > > > Kowshik
> > > > > > > >
> > > > > > > > On Mon, Apr 6, 2020 at 12:06 PM Jun Rao <j...@confluent.io>
> > > wrote:
> > > > > > > >
> > > > > > > > > Hi, Kowshik,
> > > > > > > > >
> > > > > > > > > Thanks for the reply. A few more replies below.
> > > > > > > > >
> > > > > > > > > 100.6 You can look for the sentence "This operation
> requires
> > > > ALTER
> > > > > on
> > > > > > > > > CLUSTER." in KIP-455. Also, you can check its usage in
> > > > > > > > > KafkaApis.authorize().
> > > > > > > > >
> > > > > > > > > 110. From the external client/tooling perspective, it's
> more
> > > > > natural
> > > > > > to
> > > > > > > > use
> > > > > > > > > the release version for features. If we can use the same
> > > release
> > > > > > > version
> > > > > > > > > for internal representation, it seems simpler (easier to
> > > > > understand,
> > > > > > no
> > > > > > > > > mapping overhead, etc). Is there a benefit with separate
> > > external
> > > > > and
> > > > > > > > > internal versioning schemes?
> > > > > > > > >
> > > > > > > > > 111. To put this in context, when we had IBP, the default
> > value
> > > > is
> > > > > > the
> > > > > > > > > current released version. So, if you are a brand new user,
> > you
> > > > > don't
> > > > > > > need
> > > > > > > > > to configure IBP and all new features will be immediately
> > > > available
> > > > > > in
> > > > > > > > the
> > > > > > > > > new cluster. If you are upgrading from an old version, you
> do
> > > > need
> > > > > to
> > > > > > > > > understand and configure IBP. I see a similar pattern here
> > for
> > > > > > > > > features. From the ease of use perspective, ideally, we
> > > shouldn't
> > > > > > > > require a
> > > > > > > > > new user to have an extra step such as running a bootstrap
> > > script
> > > > > > > unless
> > > > > > > > > it's truly necessary. If someone has a special need (all
> the
> > > > cases
> > > > > > you
> > > > > > > > > mentioned seem special cases?), they can configure a mode
> > such
> > > > that
> > > > > > > > > features are enabled/disabled manually.
> > > > > > > > >
> > > > > > > > > Jun
> > > > > > > > >
> > > > > > > > > On Fri, Apr 3, 2020 at 5:54 PM Kowshik Prakasam <
> > > > > > > kpraka...@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Jun,
> > > > > > > > > >
> > > > > > > > > > Thanks for the feedback and suggestions. Please find my
> > > > response
> > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > (Kowshik): I don't see any reference to the words
> > > ResourceType
> > > > or
> > > > > > > > > > AclOperations
> > > > > > > > > > in the KIP. Please let me know how I can use the KIP that
> > you
> > > > > > linked
> > > > > > > to
> > > > > > > > > > know how to
> > > > > > > > > > setup the appropriate ResourceType and/or
> ClusterOperation?
> > > > > > > > > >
> > > > > > > > > > > 105. If we change delete to disable, it's better to do
> > this
> > > > > > > > > consistently
> > > > > > > > > > in
> > > > > > > > > > > request protocol and admin api as well.
> > > > > > > > > >
> > > > > > > > > > (Kowshik): The API shouldn't be called 'disable' when it
> is
> > > > > > deleting
> > > > > > > a
> > > > > > > > > > feature.
> > > > > > > > > > I've just changed the KIP to use 'delete'. I don't have a
> > > > strong
> > > > > > > > > > preference.
> > > > > > > > > >
> > > > > > > > > > > 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?
> > > > > > > > > >
> > > > > > > > > > (Kowshik): The release version can be mapped to a set of
> > > > feature
> > > > > > > > > versions,
> > > > > > > > > > and this can be done, for example in the tool (or even
> > > external
> > > > > to
> > > > > > > the
> > > > > > > > > > tool).
> > > > > > > > > > Can you please clarify what I'm missing?
> > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > (Kowshik): I agree that there is a trade-off here, but it
> > > will
> > > > > help
> > > > > > > > > > to decide whether the automation can be thought through
> in
> > > the
> > > > > > future
> > > > > > > > > > in a follow up KIP, or right now in this KIP. We may
> invest
> > > > > > > > > > in automation, but we have to decide whether we should do
> > it
> > > > > > > > > > now or later.
> > > > > > > > > >
> > > > > > > > > > For the inconvenience that you mentioned, do you think
> the
> > > > > problem
> > > > > > > that
> > > > > > > > > you
> > > > > > > > > > mentioned can be  overcome by asking for the cluster
> > operator
> > > > to
> > > > > > run
> > > > > > > a
> > > > > > > > > > bootstrap script  when he/she knows that a specific AK
> > > release
> > > > > has
> > > > > > > been
> > > > > > > > > > almost completely deployed in a cluster for the first
> time?
> > > > Idea
> > > > > is
> > > > > > > > that
> > > > > > > > > > the
> > > > > > > > > > bootstrap script will know how to map a specific AK
> release
> > > to
> > > > > > > > finalized
> > > > > > > > > > feature versions, and run the `kafka-features.sh` tool
> > > > > > appropriately
> > > > > > > > > > against
> > > > > > > > > > the cluster.
> > > > > > > > > >
> > > > > > > > > > Now, coming back to your automation proposal/question.
> > > > > > > > > > I do see the value of automated feature version
> > finalization,
> > > > > but I
> > > > > > > > also
> > > > > > > > > > see
> > > > > > > > > > that this will open up several questions and some risks,
> as
> > > > > > explained
> > > > > > > > > > below.
> > > > > > > > > > The answers to these depend on the definition of the
> > > automation
> > > > > we
> > > > > > > > choose
> > > > > > > > > > to build, and how well does it fit into a kafka
> deployment.
> > > > > > > > > > Basically, it can be unsafe for the controller to
> finalize
> > > > > feature
> > > > > > > > > version
> > > > > > > > > > upgrades automatically, without learning about the intent
> > of
> > > > the
> > > > > > > > cluster
> > > > > > > > > > operator.
> > > > > > > > > > 1. We would sometimes want to lock feature versions only
> > when
> > > > we
> > > > > > have
> > > > > > > > > > externally verified
> > > > > > > > > > the stability of the broker binary.
> > > > > > > > > > 2. Sometimes only the cluster operator knows that a
> cluster
> > > > > upgrade
> > > > > > > is
> > > > > > > > > > complete,
> > > > > > > > > > and new brokers are highly unlikely to join the cluster.
> > > > > > > > > > 3. Only the cluster operator knows that the intent is to
> > > deploy
> > > > > the
> > > > > > > > same
> > > > > > > > > > version
> > > > > > > > > > of the new broker release across the entire cluster (i.e.
> > the
> > > > > > latest
> > > > > > > > > > downloaded version).
> > > > > > > > > > 4. For downgrades, it appears the controller still needs
> > some
> > > > > > > external
> > > > > > > > > > input
> > > > > > > > > > (such as the proposed tool) to finalize a feature version
> > > > > > downgrade.
> > > > > > > > > >
> > > > > > > > > > If we have automation, that automation can end up failing
> > in
> > > > some
> > > > > > of
> > > > > > > > the
> > > > > > > > > > cases
> > > > > > > > > > above. Then, we need a way to declare that the cluster is
> > > "not
> > > > > > ready"
> > > > > > > > if
> > > > > > > > > > the
> > > > > > > > > > controller cannot automatically finalize some basic
> > required
> > > > > > feature
> > > > > > > > > > version
> > > > > > > > > > upgrades across the cluster. We need to make the cluster
> > > > operator
> > > > > > > aware
> > > > > > > > > in
> > > > > > > > > > such a scenario (raise an alert or alike).
> > > > > > > > > >
> > > > > > > > > > > 112. DeleteFeaturesResponse: It seems the apiKey should
> > be
> > > 49
> > > > > > > instead
> > > > > > > > > of
> > > > > > > > > > 48.
> > > > > > > > > >
> > > > > > > > > > (Kowshik): Done.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Cheers,
> > > > > > > > > > Kowshik
> > > > > > > > > >
> > > > > > > > > > On Fri, Apr 3, 2020 at 11:24 AM Jun Rao <
> j...@confluent.io>
> > > > > 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.
> > > > > > > > > > >
> > > > > > > > > > > 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
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-584: Versioning scheme for features

Reply via email to