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