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