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 > > > > > > > > > >>>>>> > > > > > > > > > >>>>> > > > > > > > > > >>>> > > > > > > > > > >>> > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
