Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

Hi Tina,

Great stuff, LGTM.

Cheers,

Chris

On Mon, Feb 20, 2023 at 8:12 AM Gantigmaa Selenge 
wrote:

> Hi Chris,
>
> I have fixed the nits you mentioned and tried to improve the flow a little
> bit.
>
> Please let me know what you think :) .
>
> Thank you.
> Regards,
> Tina
>
> On Fri, Feb 17, 2023 at 6:15 PM Chris Egerton 
> wrote:
>
> > 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 
> > 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  >
> > > 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 a

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

Hi Chris,

I have fixed the nits you mentioned and tried to improve the flow a little
bit.

Please let me know what you think :) .

Thank you.
Regards,
Tina

On Fri, Feb 17, 2023 at 6:15 PM Chris Egerton 
wrote:

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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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 
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 
> 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 
> > 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=72 --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=72 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:
> > > retention.ms=72}
> > >
> > >
> > > ./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=6048

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

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 
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 
> 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=72 --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=72 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:
> > retention.ms=72}
> >
> >
> > ./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=60480 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 top

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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 
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=72 --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=72 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:
> retention.ms=72}
>
>
> ./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=60480 sensitive=false synonyms={}
>
>
> Therefore, even with the legacy API, the deletion of the source topic does
> not actually get propagated because isDefault() check here
> 
> 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  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 n

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

Hi Tina,

Thanks for the updates!

RE item 1: Ah, to be clear, I wasn't suggesting that we retry with the
incremental API for every request, just wondering about whether we'd want
to block on the very first request completing before we issue subsequent
requests, since otherwise we might send multiple requests with an
unsupported API to the broker. I don't necessarily foresee any issues if
this does happen, but it seemed worth calling that detail out in case there
are implications from it that may catch others' eyes. Anyways, no further
questions on this point--I think it's fine to potentially issue those
subsequent requests (unless anyone else finds something wrong with them),
and I also think it's fine to require users to restart/reconfigure MM2 in
order for it to go back to using the incremental API in the event that it
was originally configured with "use.incremental.alter.configs" set to
"requested" and then had to fall back on the legacy API due to
incompatibilities with the broker.

RE item 2:

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

Is that correct? I was under the impression that the source does get reset,
and local tests using the kafka-configs.sh script and the Java Admin client
both seem to confirm this (I verified by using the Admin::describeConfigs
method [1], which is what MM2 currently uses to scan for configs from
topics on the source cluster [2]).

Based on the above, I think this part is inaccurate:

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

I think the behavior when MM2 users the legacy API is actually that configs
which are deleted from topics on the source cluster are reset to the target
cluster's default on the target cluster. If MM2 uses the incremental API,
we run the risk of leaving the property unchanged on the cluster, and not
propagating the deletion--just as you'd imagined, initially. One option
could be to act differently depending on which API we're using for the
topic config alteration request, in order to preserve the same user-facing
behavior:

- If we're using the legacy API, keep the existing behavior, where we never
propagate default properties from the source to the target (which
effectively wipes them from the target, due to the all-or-nothing nature of
the legacy API)
- If we're using the incremental API, for any default topic config
properties on the source cluster, manually wipe them from the target
cluster, which also matches existing behavior

It'd be nice if someone more familiar with these client/broker APIs could
keep me honest; I may be misunderstanding some of the contracts here.
However, if this is all valid, we could then add the
"shouldReplicateSourceDefault" method to the ConfigPropertyFilter interface
just as suggested, but with a default of false, instead of true. It might
even be worth adding a property to the DefaultConfigPropertyFilter class
[3] that allows users to tweak this either on a per-property, property
regex, or all-or-nothing basis. One more thought--could it also make sense
to inform the ConfigPropertyFilter interface about what kind of broker API
we're using for the request?

RE item 3: LGTM, thank you :)

[1] -
https://kafka.apache.org/34/javadoc/org/apache/kafka/clients/admin/Admin.html#describeConfigs(java.util.Collection)
[2] -
https://github.com/apache/kafka/blob/eee2bf9686db85514c474732874d14456ae96ebc/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L503
[3] -
https://github.com/apache/kafka/blob/eee2bf9686db85514c474732874d14456ae96ebc/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/DefaultConfigPropertyFilter.java

Cheers,

Chris

On Mon, Feb 13, 2023 at 3:16 AM Gantigmaa Selenge 
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 ch

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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=72 --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=72 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:
retention.ms=72}


./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=60480 sensitive=false synonyms={}


Therefore, even with the legacy API, the deletion of the source topic does
not actually get propagated because isDefault() check here

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  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 
> wrote:
>
> > Hi Tina,
> >
> > Thanks for the KIP! I recently ran into this exact issue and it's great

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Luke Chen]

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
)
to list the javadoc and the method name together?

Thank you.
Luke


On Fri, Feb 3, 2023 at 11:46 PM Chris Egerton 
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 
> 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 
> > 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 
> 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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Chris Egerton]

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 
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 
> 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  wrote:
> >
> >> Hi Tina,
> >>
> >> See below.
> >>
> >> On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge 
> >> 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
> >>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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 
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  wrote:
>
>> Hi Tina,
>>
>> See below.
>>
>> On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge 
>> 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
>>
>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

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

> Hi Tina,
>
> See below.
>
> On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge 
> 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
>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Ismael Juma]

Hi Tina,

See below.

On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge 
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

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Federico Valeri]

On Wed, Jan 11, 2023 at 12:03 PM Gantigmaa Selenge  wrote:
>
> Thank you both for the feedback.
>
> > Have we considered using incremental alter configs by
> default and fallback to the legacy one if the former is unavailable?
> Initially having a flag with just on and off switches seemed simpler and it
> gives users control and awareness of what's being used. However, now I
> think using incremental alter configs by default and if it is unavailable,
> fallback to the legacy API is a good idea.
>
> > The config could have 3 possible values: requested, required, never. The
> default would be requested.
>
> 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.
>
> 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.  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.

Hi Tina, I'm in favor of this variant. Maybe the warning message could
just be "The target cluster  is not compatible with the
IncrementalAlterConfigs API, falling back to AlterConfigs API".

Thanks.

>
> > 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.
>
> Regards,
> Tina
>
> On Sat, Jan 7, 2023 at 7:45 PM Ismael Juma  wrote:
>
> > Hi,
> >
> > Thanks for the KIP. Have we considered using incremental alter configs by
> > default and fallback to the legacy one if the former is unavailable?
> >
> > The config could have 3 possible values: requested, required, never. The
> > default would be requested. In 4.0, this could would be removed and it
> > would effectively be required.
> >
> > Ismael
> >
> > On Sat, Jan 7, 2023, 10:03 AM John Roesler  wrote:
> >
> > > Hi Tina,
> > >
> > > Thanks for the KIP!
> > >
> > > I hope someone with prior MM or Kafka config experience is able to chime
> > > in here; I have neither.
> > >
> > > I took a look at your KIP, and it makes sense to me. I also think your
> > > migration plan is a good one.
> > >
> > > 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.
> > >
> > > Clearly, this will prolong the deprecation period, with implications on
> > > code maintenance, so there is some downside. But generally, I've found
> > > going above and beyond to support smooth upgrades for users to be well
> > > worth it in the long run.
> > >
> > > Thanks again,
> > > -John
> > >
> > >
> > > On Fri, Jan 6, 2023, at 05:49, Gantigmaa Selenge wrote:
> > > > Hi everyone,
> > > >
> > > > I would like to start a discussion on the MirrorMaker update that
> > > > proposes
> > > > replacing the deprecated alterConfigs API with the
> > > > incrementalAlterConfigs
> > > > API for syncing topic configurations. Please take a look at the
> > proposal
> > > > here:
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations
> > > >
> > > >
> > > > Regards,
> > > > Tina
> > >
> >

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Colin McCabe]

On Sat, Jan 7, 2023, at 11:39, Ismael Juma wrote:
> Hi,
>
> Thanks for the KIP. Have we considered using incremental alter configs by
> default and fallback to the legacy one if the former is unavailable?
>
> The config could have 3 possible values: requested, required, never. The
> default would be requested. In 4.0, this could would be removed and it
> would effectively be required.
>
> Ismael

+1

Colin

>
> On Sat, Jan 7, 2023, 10:03 AM John Roesler  wrote:
>
>> Hi Tina,
>>
>> Thanks for the KIP!
>>
>> I hope someone with prior MM or Kafka config experience is able to chime
>> in here; I have neither.
>>
>> I took a look at your KIP, and it makes sense to me. I also think your
>> migration plan is a good one.
>>
>> 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.
>>
>> Clearly, this will prolong the deprecation period, with implications on
>> code maintenance, so there is some downside. But generally, I've found
>> going above and beyond to support smooth upgrades for users to be well
>> worth it in the long run.
>>
>> Thanks again,
>> -John
>>
>>
>> On Fri, Jan 6, 2023, at 05:49, Gantigmaa Selenge wrote:
>> > Hi everyone,
>> >
>> > I would like to start a discussion on the MirrorMaker update that
>> > proposes
>> > replacing the deprecated alterConfigs API with the
>> > incrementalAlterConfigs
>> > API for syncing topic configurations. Please take a look at the proposal
>> > here:
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations
>> >
>> >
>> > Regards,
>> > Tina
>>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

Thank you both for the feedback.

> Have we considered using incremental alter configs by
default and fallback to the legacy one if the former is unavailable?
Initially having a flag with just on and off switches seemed simpler and it
gives users control and awareness of what's being used. However, now I
think using incremental alter configs by default and if it is unavailable,
fallback to the legacy API is a good idea.

> The config could have 3 possible values: requested, required, never. The
default would be requested.

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.

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

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

Regards,
Tina

On Sat, Jan 7, 2023 at 7:45 PM Ismael Juma  wrote:

> Hi,
>
> Thanks for the KIP. Have we considered using incremental alter configs by
> default and fallback to the legacy one if the former is unavailable?
>
> The config could have 3 possible values: requested, required, never. The
> default would be requested. In 4.0, this could would be removed and it
> would effectively be required.
>
> Ismael
>
> On Sat, Jan 7, 2023, 10:03 AM John Roesler  wrote:
>
> > Hi Tina,
> >
> > Thanks for the KIP!
> >
> > I hope someone with prior MM or Kafka config experience is able to chime
> > in here; I have neither.
> >
> > I took a look at your KIP, and it makes sense to me. I also think your
> > migration plan is a good one.
> >
> > 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.
> >
> > Clearly, this will prolong the deprecation period, with implications on
> > code maintenance, so there is some downside. But generally, I've found
> > going above and beyond to support smooth upgrades for users to be well
> > worth it in the long run.
> >
> > Thanks again,
> > -John
> >
> >
> > On Fri, Jan 6, 2023, at 05:49, Gantigmaa Selenge wrote:
> > > Hi everyone,
> > >
> > > I would like to start a discussion on the MirrorMaker update that
> > > proposes
> > > replacing the deprecated alterConfigs API with the
> > > incrementalAlterConfigs
> > > API for syncing topic configurations. Please take a look at the
> proposal
> > > here:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations
> > >
> > >
> > > Regards,
> > > Tina
> >
>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Ismael Juma]

Hi,

Thanks for the KIP. Have we considered using incremental alter configs by
default and fallback to the legacy one if the former is unavailable?

The config could have 3 possible values: requested, required, never. The
default would be requested. In 4.0, this could would be removed and it
would effectively be required.

Ismael

On Sat, Jan 7, 2023, 10:03 AM John Roesler  wrote:

> Hi Tina,
>
> Thanks for the KIP!
>
> I hope someone with prior MM or Kafka config experience is able to chime
> in here; I have neither.
>
> I took a look at your KIP, and it makes sense to me. I also think your
> migration plan is a good one.
>
> 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.
>
> Clearly, this will prolong the deprecation period, with implications on
> code maintenance, so there is some downside. But generally, I've found
> going above and beyond to support smooth upgrades for users to be well
> worth it in the long run.
>
> Thanks again,
> -John
>
>
> On Fri, Jan 6, 2023, at 05:49, Gantigmaa Selenge wrote:
> > Hi everyone,
> >
> > I would like to start a discussion on the MirrorMaker update that
> > proposes
> > replacing the deprecated alterConfigs API with the
> > incrementalAlterConfigs
> > API for syncing topic configurations. Please take a look at the proposal
> > here:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations
> >
> >
> > Regards,
> > Tina
>

Re: [DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[John Roesler]

Hi Tina,

Thanks for the KIP!

I hope someone with prior MM or Kafka config experience is able to chime in 
here; I have neither.

I took a look at your KIP, and it makes sense to me. I also think your 
migration plan is a good one.

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.

Clearly, this will prolong the deprecation period, with implications on code 
maintenance, so there is some downside. But generally, I've found going above 
and beyond to support smooth upgrades for users to be well worth it in the long 
run.

Thanks again,
-John


On Fri, Jan 6, 2023, at 05:49, Gantigmaa Selenge wrote:
> Hi everyone,
>
> I would like to start a discussion on the MirrorMaker update that 
> proposes
> replacing the deprecated alterConfigs API with the 
> incrementalAlterConfigs
> API for syncing topic configurations. Please take a look at the proposal
> here:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations
>
>
> Regards,
> Tina

[DISCUSS] KIP-894: Use incrementalAlterConfigs API for syncing topic configurations

[Gantigmaa Selenge]

Hi everyone,

I would like to start a discussion on the MirrorMaker update that proposes
replacing the deprecated alterConfigs API with the incrementalAlterConfigs
API for syncing topic configurations. Please take a look at the proposal
here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations


Regards,
Tina