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