Hi Mickael, > - I'm still a bit uneasy with the overlap between these 2 methods as currently `ReplicationPolicy.isInternalTopic` already handles MM2 internal topics. Should we make it only handle Kafka internal topics and `isMM2InternalTopic()` only handle MM2 topics? We can clear the overlap by making`ReplicationPolicy.isInternalTopic` handling only Kafka internal topics ( topics start with __), while `InternalTopicPolicy .isMM2InternalTopic` will handle any internal topic created by MM2.
>- I'm not sure I understand what this method is used for. There are no such methods for the other 2 topics (offset-sync and heartbeat). Also what happens if there are other MM2 instances using different naming schemes in the same cluster. Do all instances have to know about the other naming schemes? What are the expected issues if they don't? Your first comment was right, it will detect the checkpoint configure by only this MM2 instance so you are right we can work without it. (sorry for the confusion) >- RemoteClusterUtils is a client-side utility so it does not have access to the MM2 configuration. Since this new API can affect the name of the checkpoint topic, it will need to be used client-side too so users can find the checkpoint topic name. I had to realized this was the case. am not sure I got this, RemoteClusterUtils is using `MirrorClient `which have `MirrorClientConfig` that contains `REPLICATION_POLICY_CLASS` how is this different from adding `INTERNAL_TOPIC_NAMING_POLICY_CLASS`? Thanks On Fri, Feb 19, 2021 at 2:31 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi Omnia, > > Thanks for the clarifications. > > - I'm still a bit uneasy with the overlap between these 2 methods as > currently `ReplicationPolicy.isInternalTopic` already handles MM2 > internal topics. Should we make it only handle Kafka internal topics > and `isMM2InternalTopic()` only handle MM2 topics? > > - I'm not sure I understand what this method is used for. There are no > such methods for the other 2 topics (offset-sync and heartbeat). Also > what happens if there are other MM2 instances using different naming > schemes in the same cluster. Do all instances have to know about the > other naming schemes? What are the expected issues if they don't? > > - RemoteClusterUtils is a client-side utility so it does not have > access to the MM2 configuration. Since this new API can affect the > name of the checkpoint topic, it will need to be used client-side too > so users can find the checkpoint topic name. I had to realized this > was the case. > > Thanks > > On Mon, Feb 15, 2021 at 9:33 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > wrote: > > > > Hi Mickael, did you have some time to check my answer? > > > > On Thu, Jan 21, 2021 at 10:10 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > wrote: > >> > >> Hi Mickael, > >> Thanks for taking another look into the KIP, regards your questions > >> > >> - I believe we need both "isMM2InternalTopic" and > `ReplicationPolicy.isInternalTopic` as `ReplicationPolicy.isInternalTopic` > does check if a topic is Kafka internal topic, while `isMM2InternalTopic` > is just focusing if a topic is MM2 internal topic or not(which is > heartbeat/checkpoint/offset-sync). The fact that the default for MM2 > internal topics matches "ReplicationPolicy.isInternalTopic" will not be an > accurate assumption anymore once we implement this KIP. > >> > >> - "isCheckpointTopic" will detect all checkpoint topics for all MM2 > instances this is needed for "MirrorClient.checkpointTopics" which > originally check if the topic name ends with CHECKPOINTS_TOPIC_SUFFIX. So > this method just to keep the same functionality that originally exists in > MM2 > >> > >> - "checkpointTopic" is used in two places 1. At topic creation in > "MirrorCheckpointConnector.createInternalTopics" which use > "sourceClusterAlias() + CHECKPOINTS_TOPIC_SUFFIX" and 2. At > "MirrorClient.remoteConsumerOffsets" which is called by > "RemoteClusterUtils.translateOffsets" the cluster alias here referred to > as "remoteCluster" where the topic name is "remoteClusterAlias + > CHECKPOINTS_TOPIC_SUFFIX" (which is an argument in RemoteClusterUtils, not > a config) This why I called the variable cluster instead of source and > instead of using the config to figure out the cluster aliases from config > as we use checkpoints to keep `RemoteClusterUtils` compatible for existing > users. I see a benefit of just read the config a find out the cluster > aliases but on the other side, I'm not sure why "RemoteClusterUtils" > doesn't get the name of the cluster from the properties instead of an > argument, so I decided to keep it just for compatibility. > >> > >> Hope these answer some of your concerns. > >> Best > >> Omnia > >> > >> On Thu, Jan 21, 2021 at 3:37 PM Mickael Maison < > mickael.mai...@gmail.com> wrote: > >>> > >>> Hi Omnia, > >>> > >>> Thanks for the updates. Sorry for the delay but I have a few more > >>> small questions about the API: > >>> - Do we really need "isMM2InternalTopic()"? There's already > >>> "ReplicationPolicy.isInternalTopic()". If so, we need to explain the > >>> difference between these 2 methods. > >>> > >>> - Is "isCheckpointTopic()" expected to detect all checkpoint topics > >>> (for all MM2 instances) or only the ones for this connector instance. > >>> If it's the later, I wonder if we could do without the method. As this > >>> interface is only called by MM2, we could first call > >>> "checkpointTopic()" and check if that's equal to the topic we're > >>> checking. If it's the former, we don't really know topic names other > >>> MM2 instances may be using! > >>> > >>> - The 3 methods returning topic names have different APIs: > >>> "heartbeatsTopic()" takes no arguments, "offsetSyncTopic()" takes the > >>> target cluster alias and "checkpointTopic()" takes "clusterAlias" > >>> (which one is it? source or target?). As the interface extends > >>> Configurable, maybe we could get rid of all the arguments and use the > >>> config to find the cluster aliases. WDYT? > >>> > >>> These are minor concerns, just making sure I fully understand how the > >>> API is expected to be used. Once these are cleared, I'll be happy to > >>> vote for this KIP. > >>> > >>> Thanks > >>> > >>> On Fri, Jan 8, 2021 at 12:06 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> > wrote: > >>> > > >>> > Hi Mickael, > >>> > Did you get time to review the changes to the KIP? If you okay with > it could you vote for the KIP here ttps:// > www.mail-archive.com/dev@kafka.apache.org/msg113575.html? > >>> > Thanks > >>> > > >>> > On Thu, Dec 10, 2020 at 2:19 PM Omnia Ibrahim < > o.g.h.ibra...@gmail.com> wrote: > >>> >> > >>> >> Hi Mickael, > >>> >> 1) That's right the interface and default implementation will in > mirror-connect > >>> >> 2) Renaming the interface should be fine too especially if you > planning to move other functionality related to the creation there, I can > edit this > >>> >> > >>> >> if you are okay with that please vote for the KIP here > https://www.mail-archive.com/dev@kafka.apache.org/msg113575.html > >>> >> > >>> >> > >>> >> Thanks > >>> >> Omnia > >>> >> On Thu, Dec 10, 2020 at 12:58 PM Mickael Maison < > mickael.mai...@gmail.com> wrote: > >>> >>> > >>> >>> Hi Omnia, > >>> >>> > >>> >>> Thank you for the reply, it makes sense. > >>> >>> > >>> >>> A couple more comments: > >>> >>> > >>> >>> 1) I'm assuming the new interface and default implementation will > be > >>> >>> in the mirror-client project? as the names of some of these topics > are > >>> >>> needed by RemoteClusterUtils on the client-side. > >>> >>> > >>> >>> 2) I'm about to open a KIP to specify where the offset-syncs topic > is > >>> >>> created by MM2. In restricted environments, we'd prefer MM2 to only > >>> >>> have read access to the source cluster and have the offset-syncs on > >>> >>> the target cluster. I think allowing to specify the cluster where > to > >>> >>> create that topic would be a natural extension of the interface you > >>> >>> propose here. > >>> >>> > >>> >>> So I wonder if your interface could be named InternalTopicsPolicy? > >>> >>> That's a bit more generic than InternalTopicNamingPolicy. That > would > >>> >>> also match the configuration setting, internal.topics.policy.class, > >>> >>> you're proposing. > >>> >>> > >>> >>> Thanks > >>> >>> > >>> >>> On Thu, Dec 3, 2020 at 10:15 PM Omnia Ibrahim < > o.g.h.ibra...@gmail.com> wrote: > >>> >>> > > >>> >>> > Hi Mickael, > >>> >>> > Thanks for your feedback! > >>> >>> > Regards your question about having more configurations, I > considered adding > >>> >>> > configuration per each topic however this meant adding more > configurations > >>> >>> > for MM2 which already have so many, also the more complicated > and advanced > >>> >>> > replication pattern you have between clusters the more > configuration lines > >>> >>> > will be added to your MM2 config which isn't going to be pretty > if you > >>> >>> > don't have the same topics names across your clusters. > >>> >>> > > >>> >>> > Also, it added more complexity to the implementation as MM2 need > to > >>> >>> > 1- identify if a topic is checkpoints so we could list the > checkpoints > >>> >>> > topics in MirrorMaker 2 utils as one cluster could have X numbers > >>> >>> > checkpoints topics if it's connected to X clusters, this is done > right now > >>> >>> > by listing any topic with suffix `.checkpoints.internal`. This > could be > >>> >>> > done by add `checkpoints.topic.suffix` config but this would > make an > >>> >>> > assumption that checkpoints will always have a suffix also > having a suffix > >>> >>> > means that we may need a separator as well to concatenate this > suffix with > >>> >>> > a prefix to identify source cluster name. > >>> >>> > 2- identify if a topic is internal, so it shouldn't be > replicated or track > >>> >>> > checkpoints for it, right now this is relaying on disallow > topics with > >>> >>> > `.internal` suffix to be not replicated and not tracked in > checkpoints but > >>> >>> > with making topics configurable we need a way to define what is > an internal > >>> >>> > topic. This could be done by making using a list of all internal > topics > >>> >>> > have been entered to the configuration. > >>> >>> > > >>> >>> > So having an interface seemed easier and also give more > flexibility for > >>> >>> > users to define their own topics name, define what is internal > topic means, > >>> >>> > how to find checkpoints topics and it will be one line config > for each > >>> >>> > herder, also it more consistence with MM2 code as MM2 config have > >>> >>> > TopicFilter, ReplicationPolicy, GroupFilter, etc as interface > and they can > >>> >>> > be overridden by providing a custom implementation for them or > have some > >>> >>> > config that change their default implementations. > >>> >>> > > >>> >>> > Hope this answer your question. I also updated the KIP to add > this to the > >>> >>> > rejected solutions. > >>> >>> > > >>> >>> > > >>> >>> > On Thu, Dec 3, 2020 at 3:19 PM Mickael Maison < > mickael.mai...@gmail.com> > >>> >>> > wrote: > >>> >>> > > >>> >>> > > Hi Omnia, > >>> >>> > > > >>> >>> > > Thanks for the KIP. Indeed being able to configure MM2's > internal > >>> >>> > > topic names would be a nice improvement. > >>> >>> > > > >>> >>> > > Looking at the KIP, I was surprised you propose an interface > to allow > >>> >>> > > users to specify names. Have you considered making names > changeable > >>> >>> > > via configurations? If so, we should definitely mention it in > the > >>> >>> > > rejected alternatives as it's the first method that comes to > mind. > >>> >>> > > > >>> >>> > > I understand an interface gives a lot of flexibility but I'd > expect > >>> >>> > > topic names to be relatively simple and known in advance in > most > >>> >>> > > cases. > >>> >>> > > > >>> >>> > > I've not checked all use cases but something like below felt > appropriate: > >>> >>> > > clusters = primary,backup > >>> >>> > > primary->backup.offsets-sync.topic=backup.mytopic-offsets-sync > >>> >>> > > > >>> >>> > > On Tue, Dec 1, 2020 at 3:36 PM Omnia Ibrahim < > o.g.h.ibra...@gmail.com> > >>> >>> > > wrote: > >>> >>> > > > > >>> >>> > > > Hey everyone, > >>> >>> > > > Please take a look at KIP-690: > >>> >>> > > > > >>> >>> > > > > >>> >>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-690%3A+Add+additional+configuration+to+control+MirrorMaker+2+internal+topics+naming+convention > >>> >>> > > > > >>> >>> > > > Thanks for your feedback and support. > >>> >>> > > > > >>> >>> > > > Omnia > >>> >>> > > > > >>> >>> > > >
