Hi Federico, Ah yes, sorry about that! You're correct that this would keep the two classes in line and largely eliminate the concern I posed about porting changes to both. Still, I'm a bit hesitant, and I'm not actually certain that this alternative is more intuitive. The name isn't very descriptive, and this is the kind of approach we can only really take once; if we break compatibility again, would we introduce a LegacyLegacyReplicationPolicy? LegacyReplicationPolicy2? Finally, it seems a bit strange to introduce a new class to implement a change in behavior this small.
That said, I don't think this is worth blocking on and wouldn't be opposed if others felt strongly that a new replication policy class is superior to a new property on the existing class. Cheers, Chris On Wed, Jul 19, 2023 at 2:53 PM Federico Valeri <fedeval...@gmail.com> wrote: > Hi Chris, the KIP says it would be a subclass of DefaultReplicationPolicy > that overrides the ReplicationPolicy.offsetSyncsTopic and > ReplicationPolicy.checkpointsTopic. So, not much to maintain and it would > be more intuitive, as you say. > > On Wed, Jul 19, 2023, 4:50 PM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > HI all, > > > > I'm not sure I understand the benefits of introducing a separate > > replication policy class, besides maybe being more readable/intuitive to > > users who would want to know when to use one or the other. It feels like > > we've swapped out a "fix the bug" property for an entire "fix the bug" > > class, and it still leaves the problem of graceful migration from legacy > > behavior to new behavior unsolved. It would also require us to consider > > whether any future changes we make to the DefaultReplicationPolicy class > > would have to be ported over to the LegacyReplicationPolicy class as > well. > > > > Perhaps I'm missing something; are there other benefits of introducing a > > separate replication policy class? > > > > Cheers, > > > > Chris > > > > On Wed, Jul 19, 2023 at 5:45 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > > wrote: > > > > > 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 > > > > > > > > >>>>>> > > > > > > > > >>>>> > > > > > > > > >>>> > > > > > > > > >>> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
