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