Hi All, I like your idea, Victor, and I am ready to drop this KIP (or modify) in this form and focus my attention on the suggested long term solution.
> deprecate ReplicationPolicy as an interface and just replace it with > a fully configurable DefaultReplicationPolicy class where all topic names, > patterns, separators and filters can be changed If we all agree that this would be the best direction, then, I would be more than happy to implement this new approach. On Thu, Aug 8, 2024 at 2:28 PM Viktor Somogyi-Vass <viktor.somo...@cloudera.com.invalid> wrote: > Hi all, > > Regarding Omnia's last point: I also second that with a reasonable default > we won't really complicate average users' problems while setting a config > is simpler than having to provide a jar to override the replication policy. > I have also mulled over what if we created a separate replication policy to > capture this and KIP-1074 > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-1074> together but > unfortunately that'd further complicate the problem as we wouldn't have to > just set a few configs but additionally we could do it if we also > configured a new replication policy. > > Regarding long term (not in the scope of this KIP), I think the best would > be to deprecate ReplicationPolicy as an interface and just replace it with > a fully configurable DefaultReplicationPolicy class where all topic names, > patterns, separators and filters can be changed. We wouldn't lose > functionality in IdentityReplicationPolicy as it currently doesn't redefine > topic names and all that but I don't think that we need an interface here > either. > > Best, > Viktor > > On Wed, Jul 31, 2024 at 5:12 PM Kondrát Bertalan <kb.p...@gmail.com> > wrote: > > > Hi All, > > > > *Viktor * > > I was wrong sorry, Chris is right we have the per replication flow > > configuration for free. > > > > *Chris, regarding your comments.* > > *>IMO we don't need the "default." prefix* > > Sure I can do that, my intention with that was to make it clear for the > > users, that this property is not compatible with anything else but the > > DefaultReplicationPolicy > > > > *>I'm also a little hazy on the motivation for the change. Just out of* > > > > > > > > *curiosity, what exactly is meant by "the "heartbeats" topics of > > othersystems" in the Jira ticket's description? Are we trying to > > betteraccommodate cases where other harder-to-configure systems (like a > > pickysource connector, for example) * > > Exactly, we had a customer with a MongoDb connector if I recall it > > correctly, and for some reason they could not change the topic name on > > connector side. > > > > *Omania* > > You are right, I did not notice that my solution is in the rejected > > alternatives of KIP-690. I can't argue with your point, and I understand > > that the configuration of the mirror maker is not straight forward with > all > > that options. But those properties are hidden from the user with a > > reasonable default value, so in my opinion it should not confuse the > users, > > but it is there if someone want to modify it. > > Renaming an internal topic should not require implementing a new > > replication policy class and attaching it to the classpath in my opinion. > > I let you and the community decide whether is it worth to reconsider the > > previous solution. (I have updated the KIP) > > > > Thanks > > Berci > > > > On Fri, Feb 2, 2024 at 4:31 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > > wrote: > > > > > I noticed you have something addressed in the rejected alternatives in > > > your KIP regarding KIP-690, however, as the KIP is implementing one of > > the > > > previous rejected alternatives for KIP-690 I think it would be nice to > > > update the motivation section in the new KIP with some clarification > for > > > why what was mentioned in the rejected alternatives in KIP-690 isn’t > > valid > > > anymore. > > > > > > > On 2 Feb 2024, at 15:23, Omnia Ibrahim <o.g.h.ibra...@gmail.com> > > wrote: > > > > > > > > Hi > > > > Thanks for the KIP. I don’t know if you had a look before into the > > > rejection alternatives in KIP-690 > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-690%3A+Add+additional+configuration+to+control+MirrorMaker+2+internal+topics+naming+convention#KIP690:AddadditionalconfigurationtocontrolMirrorMaker2internaltopicsnamingconvention-RejectedAlternatives > > > or not but one of the things we were worried about is the amount of > > > configuration MM2 already has at the moment specially for advanced and > > > complicated replication between multiple clusters. This why we > > bin-backing > > > on the existing of replication policy configs to allow users to define > > > their own replication policy with their customised internal topics. > > > > > > > > Is the assumption now different meaning that we are okay to add more > > > configs for mm2? If so can you mention this in the KIP as it was > rejected > > > before in another KIP > > > > > > > > Also wouldn’t this mean that we now have two different ways to set > the > > > `heartbeat` topic name one by this config and another one by overriding > > the > > > replication policy class? It maybe worth mentioning in your KIP why you > > are > > > adding this other path to configure heartbeat topic with single config > > > instead of the existing route which providing replication policy with > the > > > new names of internal topics. And when users should opt-in for each one > > of > > > them. > > > > > > > > Thanks > > > > > > > >> On 22 Jan 2024, at 19:52, Chris Egerton <chr...@aiven.io.INVALID> > > > wrote: > > > >> > > > >> Hi Berci, > > > >> > > > >> Thanks for the KIP! > > > >> > > > >> IMO we don't need the "default." prefix for the new property, and it > > > >> deviates a bit from the precedent set by properties like > > > >> "replication.policy.internal.topic.separator.enabled". I think we > can > > > just > > > >> call it "replication.policy.heartbeats.topic", or if we really want > to > > > be > > > >> precise, "replication.policy.heartbeats.topic.name". > > > >> > > > >> Regarding multiple source->target pairs, won't we get support for > this > > > for > > > >> free if we add the new property to the DefaultReplicationPolicy > class? > > > IIRC > > > >> it's already possible to configure replication policies on a > > > >> per-replication-flow basis with that syntax, I don't see why this > > > wouldn't > > > >> be the case for the new property. > > > >> > > > >> I'm also a little hazy on the motivation for the change. Just out of > > > >> curiosity, what exactly is meant by "the "heartbeats" topics of > other > > > >> systems" in the Jira ticket's description? Are we trying to better > > > >> accommodate cases where other harder-to-configure systems (like a > > picky > > > >> source connector, for example) create and use a "heartbeats" topic, > or > > > are > > > >> we trying to enable multiple MM2 heartbeat connectors to target the > > same > > > >> Kafka cluster? I can understand the former as a niche but possible > > > scenario > > > >> and one that we can make room for, but the latter is a bit harder to > > > >> justify short of, e.g., fine-tuning the heartbeat emission interval > > > based > > > >> on the eventual target of the replication flow that will be reading > > from > > > >> the heartbeats topic. > > > >> > > > >> I don't raise the above to cast doubt on the KIP, really I'm just > > > curious > > > >> about how people are using MM2. > > > >> > > > >> Cheers, > > > >> > > > >> Chris > > > >> > > > >> On Thu, Jan 18, 2024 at 6:11 AM Kondrát Bertalan <kb.p...@gmail.com > > > > > wrote: > > > >> > > > >>> Hi Viktor, > > > >>> > > > >>> Let me address your points one by one. > > > >>> > > > >>> 1. The current implementation does not support the source->target > > > pair > > > >>> based configuration, it is global. > > > >>> 2. Yes, I introduced that property both in the client and in the > > > >>> connectors > > > >>> 3. This is a great idea, I am going to do that, and also I tried > to > > > >>> construct the property name in a way that makes this clear for > the > > > >>> users: ' > > > >>> default.replication.policy.heartbeats.topic.name' > > > >>> 4. Yeah, that was my impression too. > > > >>> > > > >>> Thanks, > > > >>> Berci > > > >>> > > > >>> On Wed, Jan 17, 2024 at 4:51 PM Viktor Somogyi-Vass > > > >>> <viktor.somo...@cloudera.com.invalid> wrote: > > > >>> > > > >>>> Hi Bertalan, > > > >>>> > > > >>>> Thanks for creating this KIP. > > > >>>> A couple of observations/questions: > > > >>>> 1. If I have multiple source->target pairs, can I set this > property > > > per > > > >>>> cluster by prefixing with "source->target" as many other configs > or > > > is it > > > >>>> global? > > > >>>> 2. The replication policy must be set in MirrorClient as well. Is > > your > > > >>>> change applicable to both MirrorClient and the connectors as well? > > > >>>> 3. It might be worth pointing out (both in the docs and the KIP) > > that > > > if > > > >>>> the user overrides the replication policy to any other than > > > >>>> DefaultReplicationPolicy, then this config has no effect. > > > >>>> 4. With regards to integration tests, I tend to lean towards that > we > > > >>> don't > > > >>>> need them if we can cover this well with unit tests and mocking. > > > >>>> > > > >>>> Thanks, > > > >>>> Viktor > > > >>>> > > > >>>> On Wed, Jan 17, 2024 at 12:23 AM Ryanne Dolan < > > ryannedo...@gmail.com> > > > >>>> wrote: > > > >>>> > > > >>>>> Makes sense to me, +1. > > > >>>>> > > > >>>>> On Tue, Jan 16, 2024 at 5:04 PM Kondrát Bertalan < > > kb.p...@gmail.com> > > > >>>>> wrote: > > > >>>>> > > > >>>>>> Hey Team, > > > >>>>>> > > > >>>>>> I would like to start a discussion thread about the *KIP-1016 > Make > > > MM2 > > > >>>>>> heartbeats topic name configurable > > > >>>>>> < > > > >>>>>> > > > >>>> > > > >>> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1016+Make+MM2+heartbeats+topic+name+configurable > > > >>>>>>> * > > > >>>>>> . > > > >>>>>> > > > >>>>>> This KIP aims to make the default heartbeat topic name > > > (`heartbeats`) > > > >>> in > > > >>>>>> the DefaultReplicationPolicy configurable via a property. > > > >>>>>> Since this is my first KIP and the change is small, I > implemented > > it > > > >>> in > > > >>>>>> advance so, I can include the PR > > > >>>>>> <https://github.com/apache/kafka/pull/15200> as well. > > > >>>>>> > > > >>>>>> I appreciate all your feedbacks and comments. > > > >>>>>> > > > >>>>>> Special thanks to Viktor Somogyi-Vass < > > viktor.somo...@cloudera.com> > > > >>> and > > > >>>>>> Daniel > > > >>>>>> Urban <urb.dani...@gmail.com> for the original idea and help. > > > >>>>>> Thank you, > > > >>>>>> Berci > > > >>>>>> > > > >>>>>> -- > > > >>>>>> *Bertalan Kondrat* | Founder, SWE > > > >>>>>> servy.hu <https://www.servy.hu/> > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>> <https://www.cloudera.com/> > > > >>>>>> ------------------------------ > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >>> > > > >>> -- > > > >>> *Bertalan Kondrat* | Founder > > > >>> t. +36(70) 413-4801 > > > >>> servy.hu <https://www.servy.hu/> > > > >>> > > > >>> > > > >>> [image: Servy] <https://www.cloudera.com/> > > > >>> ------------------------------ > > > >>> > > > > > > > > > > > > > > -- > > *Bertalan Kondrat* | Founder > > t. +36(70) 413-4801 > > servy.hu <https://www.servy.hu/> > > > > > > [image: Servy] <https://www.cloudera.com/> > > ------------------------------ > > > -- *Bertalan Kondrat* t. +36(70) 413-4801 ------------------------------