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