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