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