Hi Chris,
I have fixed the nits you mentioned and tried to improve the flow a little
bit.
Please let me know what you think :) .
Thank you.
Regards,
Tina
On Fri, Feb 17, 2023 at 6:15 PM Chris Egerton <chr...@aiven.io.invalid>
wrote:
> Hi Tina,
>
> This is looking great. I have a few nits remaining but apart from those I'm
> happy with the KIP and ready to vote.
>
> 1. In the description for how MM2 will behave when configured with
> "use.incremental.alter.configs" set to "requested", the KIP states that "If
> the first request receives an error from an incompatible broker, it will
> fallback to the deprecated AlterConfigs API for the subsequent calls". I
> think this should be "If any request receives an error" instead of "If the
> first request receives an error" since the first request may fail with a
> different error (temporarily unreachable broker, for example), but
> subsequent requests may reveal that the targeted cluster does not support
> the incremental API.
>
> 2. I've realized that I was imagining that
> ConfigPropertyFilter::shouldReplicateSourceDefault would accept a single
> string parameter (the name of the property to replicate) and return a
> boolean value, but this isn't actually laid out in the KIP anywhere. Can
> you include a Java snippet of the interface definition for the new method?
> It might look something like this if the behavior matches what I had in
> mind:
>
> public interface ConfigPropertyFilter extends Configurable, AutoCloseable {
> boolean shouldReplicateSourceDefault(String prop); // New method
> }
>
> 3. In the "Compatibility, Deprecation, and Migration Plan" section it's
> stated that the default value for "use.incremental.alter.configs" will be
> "required". I believe this should instead be "requested".
>
> Cheers,
>
> Chris
>
> On Fri, Feb 17, 2023 at 6:26 AM Gantigmaa Selenge <gsele...@redhat.com>
> wrote:
>
> > Hi Chris,
> >
> > > - The incremental API is used
> > - ConfigPropertyFilter::shouldReplicateConfigProperty returns true
> > - ConfigPropertyFilter::shouldReplicateSourceDefault returns false
> >
> > This sounds good to me. So just to clarify this in my head, when
> > incremental API is used, the MM2 will check shouldReplicateSourceDefault
> > first, which is false by default. When false, as you said it will
> manually
> > delete the default configs in the target cluster. When set to true, it
> will
> > include all the configs including the defaults for syncing.
> >
> > It would then check shouldReplicateConfigProperty for each config, it
> will
> > return true unless the config is specified in "config.properties.exclude"
> > property of DefaultConfigPropertyFilter.
> >
> > > I also think that extending the DefaultConfigPropertyFilter to allow
> > users
> > to control which defaults (source or target) get applied on the target
> > cluster is worth adding to the KIP. This could be as simple as adding a
> > property "use.defaults.from" with accepted values "source" and "target",
> or
> > it could be something more granular like
> > "config.properties.source.default.exclude" which, similar to the
> > "config.properties.exclude" property, could take a list of regular
> > expressions of properties whose default values should not be propagated
> > from the source cluster to the target cluster (with a default value of
> > ".*", to preserve existing behavior). I'm leaning toward keeping things
> > simple for now but both seem like viable options. And of course, if you
> > believe we should refrain from doing this, it's at least worth adding to
> > the rejected alternatives section.
> >
> > I agree with extending DefaultConfigPropertyFilter, and I would go with
> the
> > first option that adds "use.defaults.from". The user can then use the
> > existing "config.properties.exclude" property to exclude certain
> > configurations from the replication.
> >
> > I have addressed these now in the KIP.
> >
> > Regards,
> > Tina
> >
> > On Wed, Feb 15, 2023 at 5:36 PM Chris Egerton <chr...@aiven.io.invalid>
> > wrote:
> >
> > > Hi Tina,
> > >
> > > It's looking better! A few thoughts:
> > >
> > > I think we should clarify in the KIP that under these conditions, MM2
> > will
> > > explicitly wipe properties from topic configs on the target cluster
> (via
> > a
> > > DELETE operation):
> > > - The incremental API is used
> > > - ConfigPropertyFilter::shouldReplicateConfigProperty returns true
> > > - ConfigPropertyFilter::shouldReplicateSourceDefault returns false
> > >
> > > I also think that extending the DefaultConfigPropertyFilter to allow
> > users
> > > to control which defaults (source or target) get applied on the target
> > > cluster is worth adding to the KIP. This could be as simple as adding a
> > > property "use.defaults.from" with accepted values "source" and
> "target",
> > or
> > > it could be something more granular like
> > > "config.properties.source.default.exclude" which, similar to the
> > > "config.properties.exclude" property, could take a list of regular
> > > expressions of properties whose default values should not be propagated
> > > from the source cluster to the target cluster (with a default value of
> > > ".*", to preserve existing behavior). I'm leaning toward keeping things
> > > simple for now but both seem like viable options. And of course, if you
> > > believe we should refrain from doing this, it's at least worth adding
> to
> > > the rejected alternatives section.
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Wed, Feb 15, 2023 at 11:19 AM Gantigmaa Selenge <
> gsele...@redhat.com>
> > > wrote:
> > >
> > > > Thank you Chris. I agree with what you have suggested.
> > > > I have updated the KIP, please let me know if I missed anything or if
> > > there
> > > > is any other question.
> > > >
> > > > Regards,
> > > > Tina
> > > >
> > > > On Tue, Feb 14, 2023 at 4:40 PM Chris Egerton
> <chr...@aiven.io.invalid
> > >
> > > > wrote:
> > > >
> > > > > Hi Tina,
> > > > >
> > > > > While I agree that it's reasonable for users to want to favor the
> > > source
> > > > > cluster's defaults over the target cluster's, I'm hesitant to
> change
> > > this
> > > > > behavior in an opt-out fashion. IMO it's better to allow users to
> opt
> > > > into
> > > > > this (by adding a method to the ConfigPropertyFilter interface, and
> > > > > possibly extending the DefaultConfigPropertyFilter with
> configuration
> > > > > properties related to how it should handle source cluster
> defaults),
> > > but
> > > > we
> > > > > should try to preserve the existing behavior by default.
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Chris
> > > > >
> > > > > On Mon, Feb 13, 2023 at 5:10 PM Gantigmaa Selenge <
> > gsele...@redhat.com
> > > >
> > > > > wrote:
> > > > >
> > > > > > Hi Chris
> > > > > >
> > > > > > My comment on the second point is not correct. Please ignore the
> > part
> > > > > about
> > > > > > the config source (config source does set back to DEFAULT_CONFIG
> > when
> > > > > > deleting a config). I got diverted off the issue a little bit.
> > > > > >
> > > > > > With the legacy API, we do propagate deletion due to resetting
> all
> > > the
> > > > > > configs on that target topic that are not being replicated.
> However
> > > > with
> > > > > > incrementalAlterConfigs API, this changes. If we delete a config
> > that
> > > > was
> > > > > > previously altered on the source topic, the config on the target
> > > topic
> > > > is
> > > > > > left with the previous value as the default configs are not
> > > replicated.
> > > > > The
> > > > > > reason for favouring the source defaults was because it would set
> > the
> > > > > > config on the target topic with the source's default in this
> > > situation.
> > > > > >
> > > > > > Regards,
> > > > > > Tina
> > > > > >
> > > > > > On Mon, Feb 13, 2023 at 8:15 AM Gantigmaa Selenge <
> > > gsele...@redhat.com
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Chris and Luke,
> > > > > > >
> > > > > > > Thank you very much for your feedback.
> > > > > > >
> > > > > > > I have addressed some of the suggestions and would like to
> > clarify
> > > a
> > > > > few
> > > > > > > things on the others:
> > > > > > >
> > > > > > > > 1) The current logic for syncing topic configs in MM2 is
> > > basically
> > > > > > > fire-and-forget; all we do is log a warning message [1] if an
> > > attempt
> > > > > > > fails. When "use.incremental.alter.configs" is set to
> > "requested",
> > > > > we'll
> > > > > > > need to know whether attempts using the incremental API succeed
> > or
> > > > not,
> > > > > > and
> > > > > > > then adjust our behavior accordingly. Will the plan here be to
> > > block
> > > > on
> > > > > > the
> > > > > > > success/failure of the first request before sending any more,
> or
> > > will
> > > > > we
> > > > > > > just switch over to the legacy API as soon as any request fails
> > due
> > > > to
> > > > > > > targeting an incompatible broker, possibly after multiple
> > requests
> > > > with
> > > > > > the
> > > > > > > new API have been sent or at least queued up in the admin
> client?
> > > > > > >
> > > > > > > When it's set to "requested", I think the suggestion was to
> just
> > > > switch
> > > > > > > over to the legacy API as soon as any request fails due to
> > > > > > > targeting an incompatible broker. We could keep using the
> legacy
> > > API
> > > > > > until
> > > > > > > the mirrormaker setting is changed by the user or the
> mirrormaker
> > > > > > restarts
> > > > > > > instead of trying the new API every time it syncs the
> > > configurations.
> > > > > I'm
> > > > > > > not sure which approach is the best here.
> > > > > > >
> > > > > > > > 2) We don't replicate default properties from the source
> > cluster
> > > > > right
> > > > > > > now
> > > > > > > [2].
> > > > > > > Won't making ConfigPropertyFilter::shouldReplicateSourceDefault
> > > > return
> > > > > > true
> > > > > > > by default change that behavior? If so, what's the motivation
> for
> > > > > > favoring
> > > > > > > defaults from the source cluster over defaults for the target
> > > > cluster,
> > > > > > > especially given that users may already be relying on the
> > opposite?
> > > > > > >
> > > > > > > The concern was around what happens if deleting a topic config
> in
> > > > > > > the source cluster. I initially thought, because the legacy API
> > > > resets
> > > > > > all
> > > > > > > the configs other than the ones being altered, it effectively
> > > > > propagates
> > > > > > > the deletions of a config in the source cluster. I thought by
> > > > migrating
> > > > > > to
> > > > > > > incrementalAlterConfig API, the deletion would no longer get
> > > > propagated
> > > > > > but
> > > > > > > looking into it again, it may not be the case.
> > > > > > >
> > > > > > > If the user deletes a config in the source cluster, it would be
> > > reset
> > > > > to
> > > > > > > the default value but the config source does not change back to
> > > > > > > DEFAULT_CONFIG. For example:
> > > > > > >
> > > > > > > ./kafka-configs.sh --alter --entity-type topics --entity-name
> > > > > > > quickstart-events --add-config retention.ms=720000
> > > > --bootstrap-server
> > > > > > > localhost:9092
> > > > > > >
> > > > > > >
> > > > > > > /kafka-configs.sh --describe --entity-type topics --entity-name
> > > > > > > quickstart-events --bootstrap-server localhost:9092
> > > > > > >
> > > > > > > Dynamic configs for topic quickstart-events are:
> > > > > > >
> > > > > > > retention.ms=720000 sensitive=false
> > > > synonyms={DYNAMIC_TOPIC_CONFIG:
> > > > > > > retention.ms=720000}
> > > > > > >
> > > > > > >
> > > > > > > ./kafka-configs.sh --alter --entity-type topics --entity-name
> > > > > > > quickstart-events --delete-config retention.ms
> > --bootstrap-server
> > > > > > > localhost:9092
> > > > > > >
> > > > > > >
> > > > > > > /kafka-configs.sh --describe --entity-type topics --entity-name
> > > > > > > quickstart-events --bootstrap-server localhost:9092
> > > > > > >
> > > > > > > retention.ms=604800000 sensitive=false synonyms={}
> > > > > > >
> > > > > > >
> > > > > > > Therefore, even with the legacy API, the deletion of the source
> > > topic
> > > > > > does
> > > > > > > not actually get propagated because isDefault() check here
> > > > > > > <
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/kafka/blob/6e2b86597d9cd7c8b2019cffb895522deb63c93a/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L460
> > > > > > >
> > > > > > > would no longer return true. Basically, if a user deletes a
> > config
> > > > for
> > > > > > the
> > > > > > > source topic, with either API, the config on the target topic
> is
> > > > > > > overwritten with the source cluster's default. if I understood
> > this
> > > > > > > correctly, maybe we don't need to extend ConfigPropertyFilter
> as
> > it
> > > > > would
> > > > > > > still rely on DEFAULT_CONFIG config source and the KIP is not
> > > > changing
> > > > > > the
> > > > > > > current behaviour?
> > > > > > >
> > > > > > > I have addressed the last comment from Chris about excluding
> code
> > > > level
> > > > > > > details from the proposed solution.
> > > > > > >
> > > > > > > I also have addressed Luke's comment about the compatibility
> > > section.
> > > > > > >
> > > > > > > Please let me know what you think.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Tina
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Feb 7, 2023 at 8:06 AM Luke Chen <show...@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > >> Hi Tina,
> > > > > > >>
> > > > > > >> Thanks for the KIP to fix the issue.
> > > > > > >>
> > > > > > >> Some comments:
> > > > > > >> 1. In the compatibility section, you said:
> > > > > > >> `By default, the new setting will be set to false so it does
> not
> > > > > change
> > > > > > >> the
> > > > > > >> current behaviour.`
> > > > > > >>
> > > > > > >> I'm confused, what is the config we need to set to `false` to
> > > avoid
> > > > > > >> breaking compatibility?
> > > > > > >> All I can see is there is one new config introduced:
> > > > > > >> use.incremental.alter.configs, and default to "requested".
> > > > > > >> Does that mean it'll change current behavior?
> > > > > > >> If so, I think we should make it clear in the compatibility
> > > section
> > > > > > about
> > > > > > >> what will be changed after this KIP.
> > > > > > >>
> > > > > > >> 2. It looks like you're going to introduce a new method in the
> > > > > existing
> > > > > > >> interface. Could you follow the pattern in other KIP (ex:
> > KIP-888
> > > > > > >> <
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-888%3A+Batch+describe+ACLs+and+describe+client+quotas#KIP888:BatchdescribeACLsanddescribeclientquotas-NewAdminAPIs
> > > > > > >> >)
> > > > > > >> to list the javadoc and the method name together?
> > > > > > >>
> > > > > > >> Thank you.
> > > > > > >> Luke
> > > > > > >>
> > > > > > >>
> > > > > > >> On Fri, Feb 3, 2023 at 11:46 PM Chris Egerton
> > > > <chr...@aiven.io.invalid
> > > > > >
> > > > > > >> wrote:
> > > > > > >>
> > > > > > >> > Hi Tina,
> > > > > > >> >
> > > > > > >> > Thanks for the KIP! I recently ran into this exact issue and
> > > it's
> > > > > > great
> > > > > > >> to
> > > > > > >> > see a fix being proposed. I have a few small comments but
> > > overall
> > > > > this
> > > > > > >> > looks good:
> > > > > > >> >
> > > > > > >> > 1) The current logic for syncing topic configs in MM2 is
> > > basically
> > > > > > >> > fire-and-forget; all we do is log a warning message [1] if
> an
> > > > > attempt
> > > > > > >> > fails. When "use.incremental.alter.configs" is set to
> > > "requested",
> > > > > > we'll
> > > > > > >> > need to know whether attempts using the incremental API
> > succeed
> > > or
> > > > > > not,
> > > > > > >> and
> > > > > > >> > then adjust our behavior accordingly. Will the plan here be
> to
> > > > block
> > > > > > on
> > > > > > >> the
> > > > > > >> > success/failure of the first request before sending any
> more,
> > or
> > > > > will
> > > > > > we
> > > > > > >> > just switch over to the legacy API as soon as any request
> > fails
> > > > due
> > > > > to
> > > > > > >> > targeting an incompatible broker, possibly after multiple
> > > requests
> > > > > > with
> > > > > > >> the
> > > > > > >> > new API have been sent or at least queued up in the admin
> > > client?
> > > > > > >> >
> > > > > > >> > 2) We don't replicate default properties from the source
> > cluster
> > > > > right
> > > > > > >> now
> > > > > > >> > [2].
> > > > > > >> > Won't making
> > ConfigPropertyFilter::shouldReplicateSourceDefault
> > > > > return
> > > > > > >> true
> > > > > > >> > by default change that behavior? If so, what's the
> motivation
> > > for
> > > > > > >> favoring
> > > > > > >> > defaults from the source cluster over defaults for the
> target
> > > > > cluster,
> > > > > > >> > especially given that users may already be relying on the
> > > > opposite?
> > > > > > >> >
> > > > > > >> > 3) Nit: IMO the parts in the "proposed changes" section that
> > > > detail
> > > > > > >> changes
> > > > > > >> > to internal classes aren't really necessary since they're
> not
> > > > > relevant
> > > > > > >> to
> > > > > > >> > user-facing behavior and the classes/methods described in
> them
> > > > might
> > > > > > >> change
> > > > > > >> > between now and when the PR for the KIP gets
> > > > > opened/reviewed/merged. I
> > > > > > >> > think the only points that need to be in the KIP are the
> ones
> > > > > > beginning
> > > > > > >> > with "Extend ConfigPropertyFilter class", "Add a new
> > > configuration
> > > > > > >> setting
> > > > > > >> > to MirrorMaker", and "From Kafka 4.0" (which itself can just
> > > > > describe
> > > > > > >> the
> > > > > > >> > broker APIs that are used by MM2 in general, without
> referring
> > > to
> > > > > the
> > > > > > >> > specific name of the method in MM2 that will call them).
> > > > > > >> >
> > > > > > >> > [1] -
> > > > > > >> >
> > > > > > >> >
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/kafka/blob/6e2b86597d9cd7c8b2019cffb895522deb63c93a/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L429
> > > > > > >> >
> > > > > > >> > [2] -
> > > > > > >> >
> > > > > > >> >
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/kafka/blob/6e2b86597d9cd7c8b2019cffb895522deb63c93a/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L460
> > > > > > >> >
> > > > > > >> > Thanks again for the KIP!
> > > > > > >> >
> > > > > > >> > Cheers,
> > > > > > >> >
> > > > > > >> > Chris
> > > > > > >> >
> > > > > > >> > On Wed, Jan 25, 2023 at 10:44 AM Gantigmaa Selenge <
> > > > > > gsele...@redhat.com
> > > > > > >> >
> > > > > > >> > wrote:
> > > > > > >> >
> > > > > > >> > > Hi,
> > > > > > >> > >
> > > > > > >> > > If there are no further comments on the KIP, I will start
> a
> > > vote
> > > > > on
> > > > > > >> it.
> > > > > > >> > >
> > > > > > >> > > Regards,
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >> > > On Mon, Jan 16, 2023 at 11:14 AM Gantigmaa Selenge <
> > > > > > >> gsele...@redhat.com>
> > > > > > >> > > wrote:
> > > > > > >> > >
> > > > > > >> > > > Thanks everyone.
> > > > > > >> > > >
> > > > > > >> > > > I took the suggestions and updated the KIP accordingly.
> > > Please
> > > > > let
> > > > > > >> me
> > > > > > >> > > know
> > > > > > >> > > > if there is anything else I could improve on.
> > > > > > >> > > >
> > > > > > >> > > > Regards,
> > > > > > >> > > > Tina
> > > > > > >> > > >
> > > > > > >> > > > On Sun, Jan 15, 2023 at 10:24 PM Ismael Juma <
> > > > ism...@juma.me.uk
> > > > > >
> > > > > > >> > wrote:
> > > > > > >> > > >
> > > > > > >> > > >> Hi Tina,
> > > > > > >> > > >>
> > > > > > >> > > >> See below.
> > > > > > >> > > >>
> > > > > > >> > > >> On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge <
> > > > > > >> > gsele...@redhat.com>
> > > > > > >> > > >> wrote:
> > > > > > >> > > >>
> > > > > > >> > > >> > I do like this idea, however when it's set to
> > required, I
> > > > > > wasn't
> > > > > > >> > sure
> > > > > > >> > > >> how
> > > > > > >> > > >> > the mirrormaker should have. It's probably not a
> great
> > > > > > >> experience if
> > > > > > >> > > >> > mirrormaker starts crashing at some point after it's
> > > > already
> > > > > > >> running
> > > > > > >> > > >> due to
> > > > > > >> > > >> > an incompatible broker version.
> > > > > > >> > > >> >
> > > > > > >> > > >>
> > > > > > >> > > >> This would only happen if the user explicitly requests
> > the
> > > > > strict
> > > > > > >> > > required
> > > > > > >> > > >> ("non fallback") mode. There are many reasons why one
> may
> > > > want
> > > > > > >> this:
> > > > > > >> > say
> > > > > > >> > > >> you want to be sure that your system is not susceptible
> > to
> > > > the
> > > > > > >> > > >> "alterConfigs" problems or you want to write a test
> that
> > > > fails
> > > > > if
> > > > > > >> > > >> "alterConfigs' is used.
> > > > > > >> > > >>
> > > > > > >> > > >>
> > > > > > >> > > >> > If the incrementalAlterConfig request fails because
> the
> > > > > target
> > > > > > >> > cluster
> > > > > > >> > > >> is
> > > > > > >> > > >> > running an older version, then we could log a WARN
> > > message
> > > > > that
> > > > > > >> says
> > > > > > >> > > >> > something like "The config to use
> > incrementalAlterConfig
> > > > API
> > > > > > for
> > > > > > >> > > >> syncing
> > > > > > >> > > >> > topic configurations has been set to true however
> > target
> > > > > > cluster
> > > > > > >> is
> > > > > > >> > > >> running
> > > > > > >> > > >> > incompatible version therefore using the legacy
> > > alterConfig
> > > > > > API".
> > > > > > >> > This
> > > > > > >> > > >> way
> > > > > > >> > > >> > the Mirrormaker never has to stop working and makes
> the
> > > > user
> > > > > > >> aware
> > > > > > >> > of
> > > > > > >> > > >> > what's being used.
> > > > > > >> > > >>
> > > > > > >> > > >>
> > > > > > >> > > >> Logging statements are not great as the sole mechanism
> > > > > (although
> > > > > > >> > useful
> > > > > > >> > > as
> > > > > > >> > > >> a complementary one) since one cannot easily test
> against
> > > > them
> > > > > > and
> > > > > > >> > > they're
> > > > > > >> > > >> often missed alongside all the other logging
> statements.
> > > > > > >> > > >>
> > > > > > >> > > >>
> > > > > > >> > > >> > In this case, we also would not need 3 separate
> > values
> > > > > > >> > > >> > for the config, instead would use the original true
> or
> > > > false
> > > > > > >> values:
> > > > > > >> > > >> > - true - > would use incrementalAlterConfig API, but
> if
> > > > it's
> > > > > > >> > > >> unavailable,
> > > > > > >> > > >> > fallback to the legacy API
> > > > > > >> > > >> > - false -> keep using the legacy API
> > > > > > >> > > >> >
> > > > > > >> > > >> > Set the flag to true by default and remove the config
> > in
> > > > 4.0.
> > > > > > >> > > >> >
> > > > > > >> > > >>
> > > > > > >> > > >> But this means you have different behavior depending on
> > the
> > > > > > >> > > >> supported versions forever. Even though the config
> values
> > > are
> > > > > > >> simpler,
> > > > > > >> > > >> it's
> > > > > > >> > > >> harder to reason about the behavior.
> > > > > > >> > > >>
> > > > > > >> > > >> > One suggestion: I'm not sure how concerned you are
> > about
> > > > > > people's
> > > > > > >> > > >> ability
> > > > > > >> > > >> > to migrate, but if you want to make it as smooth as
> > > > possible,
> > > > > > you
> > > > > > >> > > could
> > > > > > >> > > >> add
> > > > > > >> > > >> > one more step. In the 4.0 release, while removing
> > > > > > >> > > >> > `use.incremental.alter.configs`, you can also add
> > > > > > >> > > >> > `use.legacy.alter.configs` to give users a path to
> > > continue
> > > > > > using
> > > > > > >> > the
> > > > > > >> > > >> old
> > > > > > >> > > >> > behavior even after the default changes.
> > > > > > >> > > >> >
> > > > > > >> > > >> > If we implement the fallback logic as Ismael
> suggested
> > > > > above, I
> > > > > > >> > think
> > > > > > >> > > we
> > > > > > >> > > >> > would not need this extra flag later anymore.
> > > > > > >> > > >> >
> > > > > > >> > > >> > Please let me know what you think. Then I can go
> ahead
> > > and
> > > > > > update
> > > > > > >> > the
> > > > > > >> > > >> KIP.
> > > > > > >> > > >>
> > > > > > >> > > >>
> > > > > > >> > > >> IncrementalAlterConfigs has been around since Apache
> > Kafka
> > > > 2.3,
> > > > > > >> > released
> > > > > > >> > > >> in
> > > > > > >> > > >> June 2019. By the time Apache Kafka 4.0 is released, it
> > > will
> > > > > have
> > > > > > >> been
> > > > > > >> > > at
> > > > > > >> > > >> least 4.5 years. I think it's reasonable to set it as
> the
> > > new
> > > > > > >> baseline
> > > > > > >> > > and
> > > > > > >> > > >> not maintain the fallback code. The key benefit is
> having
> > > > > > behavior
> > > > > > >> > that
> > > > > > >> > > is
> > > > > > >> > > >> easy to reason about.
> > > > > > >> > > >>
> > > > > > >> > > >> Ismael
> > > > > > >> > > >>
> > > > > > >> > > >
> > > > > > >> > >
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
