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
