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




------------------------------

Reply via email to