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
> > > > > >> > > >>
> > > > > >> > > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
