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