Hi Omnia, LGTM, thanks! We may want to note the LegacyReplicationPolicy option in the rejected alternatives section in case others prefer that option.
Given that we'd like this to land in time for 3.6.0, you may want to start a vote thread soon. Cheers, Chris On Fri, Jul 21, 2023 at 10:08 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com> wrote: > Hi Chris and Federico, > thinking about I think Chris's concern is valid and the bigger concern here > is that having this `LegacyReplicationPolicy` will eventually open the door > for diversion at some point between this `LegacyReplicationPolicy` and the > default one. > For now, let's have the flag properly fix this bug and we can keep it as an > option for people to switch between both behaviours. I know having a > bug-fix property is not great but we can treat it as a backward > compatibility property instead in order to keep old mirrors using the old > internal topics. > > Hope this is reasonable for the time being. > > Cheers, > Omnia > > On Wed, Jul 19, 2023 at 11:16 PM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > 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 > > > > > > > > > > >>>>>> > > > > > > > > > > >>>>> > > > > > > > > > > >>>> > > > > > > > > > > >>> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
