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