Hi all, 1. Yea, thinking of it more, I'm also concerned about the upgrade path and I think that it may not be worth adding a ton of code to work around that one time upgrade. It likely needs extra configs as well or documentation to instruct users, so it may not be worth the price. So let's add this to the rejected alternatives.
2. As Chris mentioned, I also feel that users may want to improve it with regexes and after a certain number of topics it may not be easily manageable. And indeed, it feels a little bit like duplicating the functionality of the topic filter. Overall I like the boolean config best as that prevents most of the unwanted replication cycles by default with the TopicFilter. Best, Viktor On Wed, Aug 7, 2024 at 10:59 AM Patrik Marton <pmar...@cloudera.com.invalid> wrote: > Hi All, > > Thanks again for your feedback! > > My thoughts on Viktor's ideas: > > 1. This would definitely be a clean solution in the end, but I also feel > that the complexity of this solution might outweigh the benefits. If you > guys think, we can give it a thought and figure out what exactly needs to > be changed and how the upgrade path would look like, but my opinion is that > since it is a rare use-case, a simple solution might be better. > > 2. I initially wanted to propose something like this, but my concern was > that by adding another way a topic can be whitelisted, we would also > increase the complexity and it would require more attention from users. But > I also think that this would probably be the safest solution of all, and > less risky than modifying regexes or using that simple boolean. > Since the TopicFilter is not really the problem as its exclude list can be > modified, and it basically acts as another layer of safety, we could > potentially add this new config (a list of topics) to the > DefaultReplicationPolicy, and any topic listed in the new config would not > be considered internal by the replication policy. This way we would not > duplicate the work of the Topic Filter, simply provide a way to tell the > DefaultReplicationPolicy that topics in this list seem internal, but they > are not. > > What do you think? > > Best, > Patrik > > > On Tue, Aug 6, 2024 at 7:17 PM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > Hi Viktor, > > > > Nice to hear from you! My thoughts: > > > > 1. I initially liked this idea, but it doesn't seem like the costs to all > > existing users outweigh the benefits that would be reaped by just a few. > > This is possibly because I'm assuming the upgrade path would be pretty > > gnarly, though; if we can make it really, really smooth, especially for > > existing users who don't need to be able to replicate topics ending in > > "-internal", then I think it'd be a viable option. > > > > 2. This is definitely safer than a simple boolean property, but doesn't > it > > feel like we're duplicating the work of the topic filter at this point, > > except with something a little more cumbersome for users? People already > > have a way of include- and exclude-listing topics by regex with that > > mechanism, and I'm worried we're not placing enough trust in the > > flexibility and safety it already provides us and are adding layers of > > safety that are ultimately redundant. I might also be pessimistic here > but > > I can't shake the feeling that we'd eventually get a KIP where someone > > states that it's too hard to add topics one-by-one and would like to > > include them by regex, prefix match, or some other broader mechanism. > > > > Do you find this convincing? I'm also curious what Greg and Patrik think. > > > > Cheers, > > > > Chris > > > > On Tue, Aug 6, 2024 at 5:08 AM Viktor Somogyi-Vass > > <viktor.somo...@cloudera.com.invalid> wrote: > > > > > Hi all, > > > > > > I think there are a couple of other alternatives to discuss. > > > > > > 1. We may just simply change the internal topic naming to something > like > > > "mm2internal" and provide upgrade functionality as well. In this > scenario > > > we wouldn't need to tinker with the prefixes or the naming patterns, we > > > just simply free up the "internal" keyword and after upgrade allow all > > > topics with internal to be mirrored. The upgrade however could be very > > > complicated as we'd need to provide a way for users to move away from > the > > > old topics (what happens in case of EOS, how do we make sure to > > discontinue > > > the old topics, what happens if the upgrade gets interrupted etc.). > But I > > > think this is worth considering and for the upgrade, we may take a few > > > ideas from the zk->kraft migration to make it easier. > > > > > > 2. A new config to force the replication of certain topics. In this, > > users > > > would have to specify topics 1 by 1 and this would bypass the filters > and > > > the replication policy. Since forcing is usually not the normal way of > > > doing things in general, users may be more willing to accept unintended > > > side effects. > > > > > > What do you all think? > > > (If they aren't better than the original ideas, I'm absolutely good > with > > > putting these to the rejected alternatives but wanted to write it all > > out.) > > > > > > Best, > > > Viktor > > > > > > On Mon, Aug 5, 2024 at 8:18 PM Chris Egerton <chr...@aiven.io.invalid> > > > wrote: > > > > > > > Hi Patrik, > > > > > > > > I think I owe you an apology. After mulling this over for a few more > > > hours > > > > and going over the code base some more, it seems like your initial > > > approach > > > > was actually better in some ways. > > > > > > > > With the regex-based approach, it becomes possible for users to > > configure > > > > the DefaultReplicationPolicy class in a way that for some topic T, > > > > isInternalTopic(T) returns false, but isCheckpointsTopic(T), > > > > isHeartbeatsTopic(T), or even isMM2InternalTopic(T) return true. It > > seems > > > > like this kind of inconsistency might cause confusion to users and > > > possibly > > > > even break things down the road depending on how we leverage these > > > methods > > > > in the future. > > > > > > > > With the initial approach, using a simple boolean to enable the > > > replication > > > > of what appear to be internal topics, there is some risk of cycles > and > > > > accidental replication of genuinely internal topics, but less than I > > > > believed at first. In order for an internal topic to be replicated, > not > > > > only would that boolean property have to be explicitly modified by > > users, > > > > but they would also have to be using a topic filter that allows those > > > > topics to pass through as well. > > > > > > > > I think a simple boolean is better here, as it's easier for users to > > > > configure, and less prone to logical inconsistencies. My only > > suggestions > > > > are: > > > > - Apply the property to the MirrorSourceConnector class instead of > the > > > > DefaultReplicationPolicy class, since we're not actually changing the > > > > definition of what is and isn't an internal topic; instead, we're > > simply > > > > allowing topics to be replicated even if they appear to be internal > > > topics > > > > - Rename it to something less verbose like > "replicate.internal.topics" > > > > - Add a warning to the docs for the property explicitly pointing > people > > > to > > > > the topic filter class, stating that they should still make sure that > > > > genuinely internal topics (instead of topics that > > > > ReplicationPolicy::isInternalTopic returns true for but which are not > > > > actually internal) are excluded by the filter > > > > > > > > Patrik, Greg, how does that sound? > > > > > > > > Best, > > > > > > > > Chris > > > > > > > > > > > > On Mon, Aug 5, 2024 at 4:34 AM Patrik Marton > > > <pmar...@cloudera.com.invalid > > > > > > > > > wrote: > > > > > > > > > Hi Greg and Chris, > > > > > > > > > > Thank you for your feedbacks! > > > > > > > > > > I added the currently existing workarounds to the “Rejected > > > Alternatives” > > > > > section, and explained why I still think we need a separate > > solution. I > > > > > also added the previous proposal to the rejected alternatives, as I > > > agree > > > > > with your concerns. > > > > > I tried to come up with a bit different regex based solution, and > > > > modified > > > > > the KIP based on that. I think it is a more straightforward way to > > get > > > > the > > > > > desired behavior. > > > > > > > > > > Let me know your thoughts. > > > > > > > > > > Thanks, > > > > > Patrik > > > > > > > > > > > > > > > > > > > > On 2024/07/30 18:57:18 Chris Egerton wrote: > > > > > > Hi Patrick, > > > > > > > > > > > > I share Greg's concerns with the feature as it's currently > > proposed. > > > I > > > > > > don't think I could vote for something unless it made replication > > of > > > > > > genuinely internal topics and replication cycles impossible, or > at > > > > least > > > > > > significantly less likely. > > > > > > > > > > > > Best, > > > > > > > > > > > > Chris > > > > > > > > > > > > On Tue, Jul 30, 2024, 14:51 Greg Harris <gr...@aiven.io.invalid> > > > > > > wrote: > > > > > > > > > > > > > Hi Patrik, > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > > > > > Your motivation for this KIP is reasonable, because it is > > > definitely > > > > > > > possible for the ".internal" suffix to collide with real > topics. > > It > > > > > would > > > > > > > have been nice if the original design included some > mm2-specific > > > > > namespace > > > > > > > like "mm2.internal" to lessen the likelihood of a collision. > > > > > > > > > > > > > > However, this is a problem that has numerous existing > > workarounds: > > > > > > > * Use a custom ReplicationPolicy and override the methods (for > > > > existing > > > > > > > workloads/mirror makers) > > > > > > > * Use non-conflicting user topic names (for new user topics) > > > > > > > * Use the replication.policy.separator to use a non-conflicting > > > > > separator > > > > > > > character (for new mirror maker setups) > > > > > > > > > > > > > > And the feature as-described has significant risks attached: > > > > > > > * May allow replication cycles and runaway replication > > > > > > > * Mirrors internal topics that are unusable on the destination > > > > cluster > > > > > > > > > > > > > > While these risks can be accounted for if a user is attentive > > (e.g. > > > > > when > > > > > > > they're writing their own ReplicationPolicy) it is not a > > risk-free > > > > > > > configuration that composes well with other out-of-the-box > > > > > configurations. > > > > > > > For example, someone may expect to take their existing > > > configuration, > > > > > turn > > > > > > > on this new option, and expect reasonable behavior, which isn't > > > > always > > > > > > > guaranteed. > > > > > > > > > > > > > > If you're still interested in this feature, please reference > the > > > > > existing > > > > > > > workarounds and include them as rejected alternatives so we can > > > know > > > > > where > > > > > > > the existing solutions fall short. > > > > > > > We'd also have to figure out if and how the risks I mentioned > > could > > > > be > > > > > > > mitigated. > > > > > > > > > > > > > > Thanks, > > > > > > > Greg > > > > > > > > > > > > > > On Tue, Jul 30, 2024 at 5:49 AM Patrik Marton > > > > > <pmar...@cloudera.com.invalid > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Team, > > > > > > > > > > > > > > > > I would like to start a discussion on KIP-1074: Make the > > > > replication > > > > > of > > > > > > > > internal topics configurable > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1074%3A+Make+the+replication+of+internal+topics+configurable > > > > > > > > > > > > > > > > > > > > > > > > > The goal of this KIP is to make it easier to replicate topics > > > that > > > > > seem > > > > > > > > internal (for example ending in .internal suffix), but are > just > > > > > normal > > > > > > > > business topics created by the user. > > > > > > > > > > > > > > > > I appreciate any feedback and recommendations! > > > > > > > > > > > > > > > > Thanks! > > > > > > > > Patrik > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
