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 <chr...@aiven.io.invalid> 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 <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 > > >> > > > > > >
