Hi, the point that the legacy approach can only be taken once is valid, so LGTM. Thanks.
Cheers Fede On Fri, Jul 21, 2023 at 4:28 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > > 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 > > > > > > > > > > > >>>>>> > > > > > > > > > > > >>>>> > > > > > > > > > > > >>>> > > > > > > > > > > > >>> > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
