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