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