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
>
