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