Hi Federico, I like the idea of implementing `LegacyReplicationPolicy` and avoiding bug fixes properties. We can drop the idea of the property and just go ahead with introducing the `LegacyReplicationPolicy` and any user upgrade from pre-KIP-690 can use this policy instead of default and no impact will happen to users upgrading from 3.x to post-KIP-949. We can mark `LegacyReplicationPolicy` as deprecated later if we want (but not in 4.0 as this is very soon) and we can drop it entirely at some point.
If there's an agreement on this approach I can upgrade the KIP. Cheers. Omnia On Wed, Jul 19, 2023 at 8:52 AM Federico Valeri <fedeval...@gmail.com> wrote: > Hi, one way to avoid the "fix the bug property" would be to provide > and document an additional LegacyReplicationPolicy, but still keeping > the current DefaultReplicationPolicy as replication.policy.class > default value, which is basically one of the workarounds suggested in > the KIP. > > On Tue, Jul 18, 2023 at 9:49 PM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > > Hi Federico/Omnia, > > > > Generally I like the idea of deprecating and eventually removing "fix the > > bug" properties like this, but 4.0 may be a bit soon. I'm also unsure of > > how we would instruct users who are relying on the property being set to > > "false" to migrate to a version of MM2 that doesn't support support it, > > beyond simply implementing their own replication policy--at which point, > > would we really be doing anyone a favor by forcing them to fork the > default > > policy just to preserve a property we removed? > > > > I guess right now I'd rather play it safe and not immediately deprecate > the > > property. If we can find an easy migration path for users who are relying > > on it, then I'd be happy to deprecate and schedule for removal. > > > > Cheers, > > > > Chris > > > > On Tue, Jul 18, 2023 at 12:54 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > > wrote: > > > > > Hi Federico, > > > I don't have any strong opinion one way or the other. The pro of > > > deprecation is not adding more configs to MM2 as it has too many > configs > > > already. However, we need to think about old MM2 migrating their > internal > > > topics to 4.0 with less impact. > > > > > > @Chris what do you think? > > > > > > Cheers > > > Omnia > > > > > > On Fri, Jul 14, 2023 at 2:38 PM Federico Valeri <fedeval...@gmail.com> > > > wrote: > > > > > > > Hi Omnia and Chris, I agree with setting > > > > "replication.policy.internal.topic.separator.enabled=true" by > default, > > > > but I was wondering if we should also deprecate and remove in Kafka > 4. > > > > If there is agreement in having the same behavior for internal and > > > > non-internal topics, then it should be fine, and we won't need to > keep > > > > an additional configuration around. Wdyt? > > > > > > > > Cheers > > > > Fede > > > > > > > > On Fri, Jul 14, 2023 at 1:51 PM Omnia Ibrahim < > o.g.h.ibra...@gmail.com> > > > > wrote: > > > > > > > > > > Hi Chris, I added a section for backport plan here > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-949%3A+Add+flag+to+enable+the+usage+of+topic+separator+in+MM2+DefaultReplicationPolicy#KIP949:AddflagtoenabletheusageoftopicseparatorinMM2DefaultReplicationPolicy-Backportingplan > > > > > > > > > > Cheers, > > > > > Omnia > > > > > > > > > > > On 13 Jul 2023, at 19:33, Chris Egerton <chr...@aiven.io.INVALID > > > > > > wrote: > > > > > > > > > > > > Hi Omnia, > > > > > > > > > > > > Yes, I think we ought to state the backport plan in the KIP, > since > > > it's > > > > > > highly unusual for KIP changes or new configuration properties > to be > > > > > > backported and we should get the approval of the community > (including > > > > > > binding and non-binding voters) before implementing it. > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Chris > > > > > > > > > > > > On Thu, Jul 13, 2023 at 7:13 AM Omnia Ibrahim < > > > o.g.h.ibra...@gmail.com > > > > > > > > > > > wrote: > > > > > > > > > > > >> Hi Chris, > > > > > >> The implementation should be very small so backporting this to > 3.1 > > > > and 3.2 > > > > > >> would be perfect for this case if you or any other committer are > > > okay > > > > with > > > > > >> approving the backporting. Do we need to state this in KIP as > well > > > or > > > > not? > > > > > >> > > > > > >> Also, I’ll open a vote for the KIP today and prepare the pr for > it > > > so > > > > we > > > > > >> can merge it as soon as we can. > > > > > >> > > > > > >> Thanks, > > > > > >> > > > > > >> Omnia > > > > > >> > > > > > >> On Wed, Jul 12, 2023 at 4:31 PM Chris Egerton > > > <chr...@aiven.io.invalid > > > > > > > > > > >> wrote: > > > > > >> > > > > > >>> Hi Omnia, > > > > > >>> > > > > > >>> Thanks for changing the default, LGTM 👍 > > > > > >>> > > > > > >>> As far as backporting goes, we probably won't be doing another > > > > release > > > > > >> for > > > > > >>> 3.1, and possibly not for 3.2 either; however, if we can make > the > > > > > >>> implementation focused enough (which I don't think would be too > > > > > >> difficult, > > > > > >>> but correct me if I'm wrong), then we can still backport > through > > > 3.1. > > > > > >> Even > > > > > >>> if we don't do another release it can make life easier for > people > > > > who are > > > > > >>> maintaining parallel forks. Obviously this shouldn't be taken > as a > > > > > >> blanket > > > > > >>> precedent but in this case it seems like the benefits may > outweigh > > > > the > > > > > >>> costs. What are your thoughts? > > > > > >>> > > > > > >>> Cheers, > > > > > >>> > > > > > >>> Chris > > > > > >>> > > > > > >>> On Wed, Jul 12, 2023 at 9:05 AM Omnia Ibrahim < > > > > o.g.h.ibra...@gmail.com> > > > > > >>> wrote: > > > > > >>> > > > > > >>>> Hi Chris, thanks for the feedback. > > > > > >>>> 1. regarding the default value I had the same conflict of > which > > > > version > > > > > >>> to > > > > > >>>> break the backward compatibility with. We can just say that > this > > > KIP > > > > > >>> gives > > > > > >>>> the release Pre KIP-690 the ability to keep the old behaviour > with > > > > one > > > > > >>>> config and keep the backwards compatibility from post-KIP-690 > the > > > > same > > > > > >> so > > > > > >>>> we don't break at least the last 3 versions. I will update > the KIP > > > > to > > > > > >>>> switch the default value to true. > > > > > >>>> 2. For the backporting, which versions can we backport these > to? > > > > > >> Usually, > > > > > >>>> Kafka supports bugfix releases as needed for the last 3 > releases. > > > > Now > > > > > >> we > > > > > >>> @ > > > > > >>>> 3.5 so the last 3 are 3.4, 3.3 and 3.2 is this correct? > > > > > >>>> 3. I'll add a Jira for updating the docs for this KIP so we > don't > > > > > >> forget > > > > > >>>> about it. > > > > > >>>> > > > > > >>>> Thanks > > > > > >>>> Omnia > > > > > >>>> > > > > > >>>> > > > > > >>>> On Mon, Jul 10, 2023 at 5:33 PM Chris Egerton > > > > <chr...@aiven.io.invalid > > > > > >>> > > > > > >>>> wrote: > > > > > >>>> > > > > > >>>>> Hi Omnia, > > > > > >>>>> > > > > > >>>>> Thanks for taking this on! I have some thoughts but the > general > > > > > >>> approach > > > > > >>>>> looks good. > > > > > >>>>> > > > > > >>>>> 1. Default value > > > > > >>>>> > > > > > >>>>> One thing I'm wrestling with is what the default value of > the new > > > > > >>>> property > > > > > >>>>> should be. I know on the Jira ticket I proposed that it > should be > > > > > >>> false, > > > > > >>>>> but I'm having second thoughts. Technically we'd preserve > > > backward > > > > > >>>>> compatibility with pre-KIP-690 releases by defaulting to > false, > > > but > > > > > >> at > > > > > >>>> the > > > > > >>>>> same time, we'd break compatibility with post-KIP-690 > releases. > > > And > > > > > >> if > > > > > >>> we > > > > > >>>>> default to true, the opposite would be true: compatibility > would > > > be > > > > > >>>> broken > > > > > >>>>> with pre-KIP-690 releases, but preserved with post-KIP-690 > > > > releases. > > > > > >>>>> > > > > > >>>>> One argument against defaulting to false (which, again, would > > > > > >> preserve > > > > > >>>> the > > > > > >>>>> behavior of MM2 before we accidentally broke compatibility > with > > > > > >>> KIP-690) > > > > > >>>> is > > > > > >>>>> that this change could possibly cause a single MM2 setup to > break > > > > > >>>>> twice--once when upgrading from a pre-KIP-690 release to an > > > > existing > > > > > >>>>> release, and again when upgrading from that existing release > to a > > > > > >>> version > > > > > >>>>> that reverted (by default) to pre-KIP-690 behavior. On the > other > > > > > >> hand, > > > > > >>> if > > > > > >>>>> we default to true (which would preserve the existing > behavior > > > that > > > > > >>>> breaks > > > > > >>>>> compatibility with pre-KIP-690 releases), then any given > setup > > > will > > > > > >>> only > > > > > >>>> be > > > > > >>>>> broken once. > > > > > >>>>> > > > > > >>>>> In addition, if we default to true right now, then we don't > have > > > to > > > > > >>> worry > > > > > >>>>> about changing that default in 4.0 to a more intuitive value > (I > > > > hope > > > > > >> we > > > > > >>>> can > > > > > >>>>> all agree that, for new clusters, it makes sense to set this > > > > property > > > > > >>> to > > > > > >>>>> true and not to distinguish between internal and non-internal > > > > > >> topics). > > > > > >>>>> > > > > > >>>>> With that in mind, I'm now leaning more towards defaulting to > > > true, > > > > > >> but > > > > > >>>>> would be interested in your thoughts. > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> 2. Backport? > > > > > >>>>> > > > > > >>>>> It's highly unlikely to backport changes for a KIP, but > given the > > > > > >>> impact > > > > > >>>> of > > > > > >>>>> the compatibility break that we're trying to address here, > and > > > the > > > > > >>>>> extremely low risk of the proposed changes, I think we should > > > > > >> consider > > > > > >>>>> backporting the proposed fix to all affected release branches > > > > (i.e., > > > > > >>> 3.1 > > > > > >>>>> through 3.5). > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> 3. Extra steps > > > > > >>>>> > > > > > >>>>> I also think we can take these additional steps to try to > help > > > > > >> prevent > > > > > >>>>> users from being bitten by this change: > > > > > >>>>> > > > > > >>>>> - Add a note to our upgrade instructions [1] for all affected > > > > > >> versions > > > > > >>>> that > > > > > >>>>> instructs users on how to safely upgrade to a post-KIP-690 > > > release, > > > > > >> for > > > > > >>>>> versions that both do and do not include the changes from > this > > > KIP > > > > > >>>>> - Log a warning message on MM2 startup if the config > contains an > > > > > >>> explicit > > > > > >>>>> value for "replication.policy.separator" but does not > contain an > > > > > >>> explicit > > > > > >>>>> value for > "replication.policy.internal.topic.separator.enabled" > > > > > >>>>> > > > > > >>>>> These details don't necessarily have to be codified in the > KIP, > > > but > > > > > >>>> they're > > > > > >>>>> worth taking into account when considering how to design any > > > > > >> functional > > > > > >>>>> changes in order to better try to gauge how well this could > go > > > for > > > > > >> our > > > > > >>>>> users. > > > > > >>>>> > > > > > >>>>> [1] - https://kafka.apache.org/documentation.html#upgrade > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> Thanks again for the KIP! > > > > > >>>>> > > > > > >>>>> Cheers, > > > > > >>>>> > > > > > >>>>> Chris > > > > > >>>>> > > > > > >>>>> On Fri, Jul 7, 2023 at 10:12 AM Omnia Ibrahim < > > > > > >> o.g.h.ibra...@gmail.com > > > > > >>>> > > > > > >>>>> wrote: > > > > > >>>>> > > > > > >>>>>> Hi everyone, > > > > > >>>>>> I want to start the discussion of the KIP-949. The proposal > is > > > > here > > > > > >>>>>> > > > > > >>>>>> > > > > > >>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-949%3A+Add+flag+to+enable+the+usage+of+topic+separator+in+MM2+DefaultReplicationPolicy > > > > > >>>>>> < > > > > > >>>>>> > > > > > >>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-949%3A+Add+flag+to+enable+the+usage+of+topic+separator+in+MM2+DefaultReplicationPolicy > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>>>> Thanks for your time and feedback. > > > > > >>>>>> Omnia > > > > > >>>>>> > > > > > >>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > >
