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