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 >
