Hi Tina, Thanks for the KIP! I recently ran into this exact issue and it's great to see a fix being proposed. I have a few small comments but overall this looks good:
1) The current logic for syncing topic configs in MM2 is basically fire-and-forget; all we do is log a warning message [1] if an attempt fails. When "use.incremental.alter.configs" is set to "requested", we'll need to know whether attempts using the incremental API succeed or not, and then adjust our behavior accordingly. Will the plan here be to block on the success/failure of the first request before sending any more, or will we just switch over to the legacy API as soon as any request fails due to targeting an incompatible broker, possibly after multiple requests with the new API have been sent or at least queued up in the admin client? 2) We don't replicate default properties from the source cluster right now [2]. Won't making ConfigPropertyFilter::shouldReplicateSourceDefault return true by default change that behavior? If so, what's the motivation for favoring defaults from the source cluster over defaults for the target cluster, especially given that users may already be relying on the opposite? 3) Nit: IMO the parts in the "proposed changes" section that detail changes to internal classes aren't really necessary since they're not relevant to user-facing behavior and the classes/methods described in them might change between now and when the PR for the KIP gets opened/reviewed/merged. I think the only points that need to be in the KIP are the ones beginning with "Extend ConfigPropertyFilter class", "Add a new configuration setting to MirrorMaker", and "From Kafka 4.0" (which itself can just describe the broker APIs that are used by MM2 in general, without referring to the specific name of the method in MM2 that will call them). [1] - https://github.com/apache/kafka/blob/6e2b86597d9cd7c8b2019cffb895522deb63c93a/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L429 [2] - https://github.com/apache/kafka/blob/6e2b86597d9cd7c8b2019cffb895522deb63c93a/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L460 Thanks again for the KIP! Cheers, Chris On Wed, Jan 25, 2023 at 10:44 AM Gantigmaa Selenge <gsele...@redhat.com> wrote: > Hi, > > If there are no further comments on the KIP, I will start a vote on it. > > Regards, > > > On Mon, Jan 16, 2023 at 11:14 AM Gantigmaa Selenge <gsele...@redhat.com> > wrote: > > > Thanks everyone. > > > > I took the suggestions and updated the KIP accordingly. Please let me > know > > if there is anything else I could improve on. > > > > Regards, > > Tina > > > > On Sun, Jan 15, 2023 at 10:24 PM Ismael Juma <ism...@juma.me.uk> wrote: > > > >> Hi Tina, > >> > >> See below. > >> > >> On Wed, Jan 11, 2023 at 3:03 AM Gantigmaa Selenge <gsele...@redhat.com> > >> wrote: > >> > >> > I do like this idea, however when it's set to required, I wasn't sure > >> how > >> > the mirrormaker should have. It's probably not a great experience if > >> > mirrormaker starts crashing at some point after it's already running > >> due to > >> > an incompatible broker version. > >> > > >> > >> This would only happen if the user explicitly requests the strict > required > >> ("non fallback") mode. There are many reasons why one may want this: say > >> you want to be sure that your system is not susceptible to the > >> "alterConfigs" problems or you want to write a test that fails if > >> "alterConfigs' is used. > >> > >> > >> > If the incrementalAlterConfig request fails because the target cluster > >> is > >> > running an older version, then we could log a WARN message that says > >> > something like "The config to use incrementalAlterConfig API for > >> syncing > >> > topic configurations has been set to true however target cluster is > >> running > >> > incompatible version therefore using the legacy alterConfig API". This > >> way > >> > the Mirrormaker never has to stop working and makes the user aware of > >> > what's being used. > >> > >> > >> Logging statements are not great as the sole mechanism (although useful > as > >> a complementary one) since one cannot easily test against them and > they're > >> often missed alongside all the other logging statements. > >> > >> > >> > In this case, we also would not need 3 separate values > >> > for the config, instead would use the original true or false values: > >> > - true - > would use incrementalAlterConfig API, but if it's > >> unavailable, > >> > fallback to the legacy API > >> > - false -> keep using the legacy API > >> > > >> > Set the flag to true by default and remove the config in 4.0. > >> > > >> > >> But this means you have different behavior depending on the > >> supported versions forever. Even though the config values are simpler, > >> it's > >> harder to reason about the behavior. > >> > >> > One suggestion: I'm not sure how concerned you are about people's > >> ability > >> > to migrate, but if you want to make it as smooth as possible, you > could > >> add > >> > one more step. In the 4.0 release, while removing > >> > `use.incremental.alter.configs`, you can also add > >> > `use.legacy.alter.configs` to give users a path to continue using the > >> old > >> > behavior even after the default changes. > >> > > >> > If we implement the fallback logic as Ismael suggested above, I think > we > >> > would not need this extra flag later anymore. > >> > > >> > Please let me know what you think. Then I can go ahead and update the > >> KIP. > >> > >> > >> IncrementalAlterConfigs has been around since Apache Kafka 2.3, released > >> in > >> June 2019. By the time Apache Kafka 4.0 is released, it will have been > at > >> least 4.5 years. I think it's reasonable to set it as the new baseline > and > >> not maintain the fallback code. The key benefit is having behavior that > is > >> easy to reason about. > >> > >> Ismael > >> > > >
