Hi Omnia, Please could you add to the KIP the documentation that forward.admin.class config will have, and include that the signature of the class named by forward.admin.class must have a constructor that takes a `Map<String, Object>`?
Many thanks, Tom On Wed, 22 Jun 2022 at 11:30, <o.g.h.ibra...@gmail.com> wrote: > Hi Tom > My initial thought was that the constructor with config and delegator > would be for testing while the one with config is the one that will be used > by MM2. > I can removed one of them and keep only one. > > Sent from my iPhone > > > On 22 Jun 2022, at 08:15, Tom Bentley <tbent...@redhat.com> wrote: > > > > Hi Omnia, > > > > Thanks for those answers. I'm a bit confused by the changes you've made > for > > 1 though. It's MM2 that's going to instantiate the class named in > > forwarding.admin.class, so it's MM2 that needs to know the constructor > > signature. The easiest way of doing this is to specify the signature as > > part of the contract for forwarding.admin.class. But ForwardingAdmin now > > has two constructors, which is confusing for someone who wants to > subclass > > it. Surely only one is needed? It would also be a good idea to show the > > documentation that forward.admin.class will have. > > > > Kind regards, > > > > Tom > > > >> On Tue, 21 Jun 2022 at 17:06, Omnia Ibrahim <o.g.h.ibra...@gmail.com> > wrote: > >> > >> Hi Tom > >> > >>> 1. I assume there's an undocumented requirement in the KIP that > whatever > >>> class is named for forwarding.admin.class it has a public single > argument > >>> constructor that takes an Admin instance? > >>> > >> you are right, I updated the KIP to reflect the missing one. > >> 3. What if the implementation needs to distinguish between creating > ACLs on > >> the source cluster and creating them on the destination cluster? E.g. > the > >> one should be done one way, but the other using a different mechanism? > >> Users will need to define 2 implementations, one for source and another > for > >> target and configure each using <cluster_alias>.forwarding.admin.class. > for > >> example > >> - target.forwarding.admin.class = TargetAdmin > >> - source.forwarding.admin.class = SourceAdmin > >> > >>> On Tue, Jun 21, 2022 at 2:14 PM Tom Bentley <tbent...@redhat.com> > wrote: > >>> > >>> Hi Omnia, > >>> > >>> Thanks for the KIP! I'm sorry for the delay in this response. I have a > >> few > >>> questions: > >>> > >>> 1. I assume there's an undocumented requirement in the KIP that > whatever > >>> class is named for forwarding.admin.class it has a public single > argument > >>> constructor that takes an Admin instance? > >>> 2. If 1 is correct then what about an implementation that requires > extra > >>> configuration, e.g. for whatever infra-as-code API it needs to use > >> (instead > >>> of using an Admin instance directly) how does it learn about that > >> non-Kafka > >>> config when it's only receiving an Admin instance? > >>> 3. What if the implementation needs to distinguish between creating > ACLs > >> on > >>> the source cluster and creating them on the destination cluster? E.g. > the > >>> one should be done one way, but the other using a different mechanism? > >>> > >>> Kind regards, > >>> > >>> Tom > >>> > >>> > >>> On Mon, 20 Jun 2022 at 17:13, Omnia Ibrahim <o.g.h.ibra...@gmail.com> > >>> wrote: > >>> > >>>> Hi Chris, sorry for the late reply. > >>>> ..Hi, > >>>> > >>>>> 1. I might be missing something, but can you give a concrete Java > >>> example > >>>>> of how the proposed ForwardingAdmin class is more convenient than > >>>>> subclassing the KafkaAdminClient class? AFAICT the two would be > >>> virtually > >>>>> identical. > >>>> > >>>> I might be misunderstanding exactly how that class will be used; > >>>>> I'm envisioning it as the pluggable class that users will implement > >> for > >>>>> custom administration logic and specify as the value for the > >>>>> "forwarding.admin.class" (or "<cluster>.forwarding.admin.class") > >>>> property, > >>>>> and that it will be instantiated with a KafkaAdminClient instance > >> that > >>>> can > >>>>> be used to get the same logic that MM2 provides today. In the case > >> you > >>>>> mentioned (KafkaAdminClient for read/describe, custom Admin for > >>>>> create/update), I'd imagine one could override the createTopics, > >>>>> deleteTopics, createAcls, deleteAcls (maybe?), alterConfigs (maybe?), > >>>> etc. > >>>>> methods, and then leave other methods such as listTopics, > >>> describeTopics, > >>>>> describeCluster, etc. as they are. > >>>>> > >>>> The Forwarding decorator is one alternative for inheritance so you are > >>>> right to say that they look identical. However, I would like to point > >> out > >>>> few points > >>>> 1. that Kafka codebase has few wrappers or decorators around > >> AdminClient > >>>> instead of inheritance so using decorator over inheritance isn't new > >>>> proposal for example > >>>> > >>>> - - > >>> `org.apache.kafka.streams.processor.internals.InternalTopicManager` > >>>> which has AdminClient as parameter instead of inheatance > >>>> - - `org.apache.kafka.connect.util.SharedTopicAdmin` and > >>>> `org.apache.kafka.connect.util.TopicAdmin` don't inherit > >>>> KafkaAdminClient > >>>> instead initialize KafkaAdminClient. > >>>> > >>>> 2. Using ForwardingAdmin will make it easier to test which methods use > >>>> KafkaAdminClient and which don't this make the test for any customized > >>>> implementation easier. We can't have this with inheatcancs > >>>> 3. Inhearting KafkaAdminClient has the following limitation > >>>> a. KafkaAdminClient doesn't have a public default constructor, > >> so > >>>> isn't gonna be easy to have contractor that initialize both > >>>> KafkaAdminClient (to be used for read/describe/list) and customized > >>>> fedrated client (to be used for create/update). (Note I don't want to > >>> touch > >>>> anything in KafkaAdminClient code base to change that) > >>>> b. the only way to initialize instance is by > >> using`createInternal` > >>>> which is static and can't be overridden to include creating customized > >>>> fedrated client. This is the method used by `Admin.create` to > >>>> initialize `KafkaAdminClient`in > >>>> most MM2 and Kafka codebase. > >>>> > >>>> 3. KIP-158 deals with the topics that source connectors write to, not > >> the > >>>>> internal topics used by the Connect framework. IIUC this includes the > >>>>> topics that MM2 mirrors. I think it's still fine if we want to leave > >>>>> support for this out and say that this KIP only addresses code that > >>> lives > >>>>> inside the connect/mirror (and possibly connect/mirror-client?) > >>> modules, > >>>> I > >>>>> just want to make sure that whatever behavior we settle on is > >> specified > >>>>> clearly in the KIP and user-facing documentation. > >>>> > >>>> MM2 deals with a large number of topics, creating the topic if not > >>>> existing, adding new partitions if the number of partitions increases > >> and > >>>> syncing configs and ACLs, so while KIP-158 can stop the source connect > >>> from > >>>> creating a topic, this wouldn't be a great solution for MM2 when it > >> runs > >>> on > >>>> a large scale as this will add too much operation headache on the > teams > >>>> that run MM2 to create/update large number of topics manually. Also, > it > >>>> prevents teams from enabling MM2 to sync configs and ACLs as this > still > >>>> will be at odds with the federated solution. > >>>> > >>>> 4. That's fine 👍 Just out of curiosity, is the motivation for this > >>>>> decision to simplify the implementation? I can imagine it'd be easier > >>> to > >>>>> modify the MM2 codebase exclusively and not have to worry about > >>> touching > >>>>> the Connect framework as well given the possibility for unintended > >>>>> consequences for other connectors with the latter. > >>>> > >>>> The motivation of the KIP is to have the option to let MM2 create > >>> topics, > >>>> add partitions, and sync configs and ACLs without being at odds with > >>>> federated or capacity solutions when it runs on a large scale instead > >> of > >>>> just turning the admin client off. > >>>> > >>>>> Wondering if there's a > >>>>> distinction on the feature front as well that makes MM2 internal > >> topics > >>>>> different from Connect internal topics. > >>>> > >>>> > >>>> The only diff here is the MM2 call TopicManager directly to create > >> these > >>>> topics. > >>>> > >>>> > >>>> Thanks > >>>> Omnia > >>>> > >>>> > >>>> On Tue, Jun 7, 2022 at 5:05 AM Chris Egerton <fearthecel...@gmail.com > > > >>>> wrote: > >>>> > >>>>> Hi Omnia, > >>>>> > >>>>> 1. I might be missing something, but can you give a concrete Java > >>> example > >>>>> of how the proposed ForwardingAdmin class is more convenient than > >>>>> subclassing the KafkaAdminClient class? AFAICT the two would be > >>> virtually > >>>>> identical. I might be misunderstanding exactly how that class will be > >>>> used; > >>>>> I'm envisioning it as the pluggable class that users will implement > >> for > >>>>> custom administration logic and specify as the value for the > >>>>> "forwarding.admin.class" (or "<cluster>.forwarding.admin.class") > >>>> property, > >>>>> and that it will be instantiated with a KafkaAdminClient instance > >> that > >>>> can > >>>>> be used to get the same logic that MM2 provides today. In the case > >> you > >>>>> mentioned (KafkaAdminClient for read/describe, custom Admin for > >>>>> create/update), I'd imagine one could override the createTopics, > >>>>> deleteTopics, createAcls, deleteAcls (maybe?), alterConfigs (maybe?), > >>>> etc. > >>>>> methods, and then leave other methods such as listTopics, > >>> describeTopics, > >>>>> describeCluster, etc. as they are. > >>>>> > >>>>> 3. KIP-158 deals with the topics that source connectors write to, not > >>> the > >>>>> internal topics used by the Connect framework. IIUC this includes the > >>>>> topics that MM2 mirrors. I think it's still fine if we want to leave > >>>>> support for this out and say that this KIP only addresses code that > >>> lives > >>>>> inside the connect/mirror (and possibly connect/mirror-client?) > >>> modules, > >>>> I > >>>>> just want to make sure that whatever behavior we settle on is > >> specified > >>>>> clearly in the KIP and user-facing documentation. > >>>>> > >>>>> 4. That's fine 👍 Just out of curiosity, is the motivation for this > >>>>> decision to simplify the implementation? I can imagine it'd be easier > >>> to > >>>>> modify the MM2 codebase exclusively and not have to worry about > >>> touching > >>>>> the Connect framework as well given the possibility for unintended > >>>>> consequences for other connectors with the latter. Wondering if > >>> there's a > >>>>> distinction on the feature front as well that makes MM2 internal > >> topics > >>>>> different from Connect internal topics. > >>>>> > >>>>> Cheers, > >>>>> > >>>>> Chris > >>>>> > >>>>> On Mon, Jun 6, 2022 at 6:36 AM Omnia Ibrahim < > >> o.g.h.ibra...@gmail.com> > >>>>> wrote: > >>>>> > >>>>>> Hi Chris, Thanks for having the time to look into this. > >>>>>> > >>>>>> 1. Is the introduction of the new "ForwardingAdmin" class > >> necessary, > >>> or > >>>>> can > >>>>>>> the same behavior can be achieved by subclassing the existing > >>>>>>> KafkaAdminClient class? > >>>>>> > >>>>>> forwarding decorators give more flexibility than the inheritance, > >> in > >>>> this > >>>>>> case, ForwardingAdmin gives the ability to use the default > >>>>> KafkaAdminClient > >>>>>> for reading/describing resources and at the same time configure > >>> another > >>>>>> client to connect to the federated solution to create and update > >>>>> resources. > >>>>>> Using forwarding seems cleaner and more flexible for this use case > >>> than > >>>>>> inheritance. > >>>>>> > >>>>>> 2. Would it be just as accurate to name the new Mirror Maker 2 > >>> property > >>>>>>> "admin.class" instead of "forwarding.admin.class"? I think > >> brevity > >>>> may > >>>>>> work > >>>>>>> in our favor here > >>>>>>> > >>>>>> I don't mind renaming it to "admin.class" if this is better. > >>>>>> > >>>>>> 3. Would the admin class specified by the user also take effect for > >>>>> KIP-158 > >>>>>>> [1] style automatic topic creation? (Forgive me if this isn't > >>>>> applicable > >>>>>>> for Mirror Maker 2; I'm asking solely based on the knowledge that > >>> MM2 > >>>>> can > >>>>>>> be run as a source connector and has its own source task class > >>> [2].) > >>>>>> > >>>>>> No this only control creating/updating mirrored topics and MM2 > >>> internal > >>>>>> topics. And not going to affect connect runtime's internal topics > >>> that > >>>>> are > >>>>>> needed by connect cluster that runs MM2. > >>>>>> > >>>>>> 4. Would the admin class specified by the user also take effect for > >>>>>>> internal topics created by the Connect framework (i.e., the > >> statue, > >>>>>> config, > >>>>>>> and offsets topics)? > >>>>>> > >>>>>> No this wouldn't address connect as connect "realtime" uses > >>> TopicAdmin > >>>>> to > >>>>>> manage topics needed for KafkaOffsetBackingStore, > >>>> KafkaStatusBackingStore > >>>>>> and KafkaConfigBackingStore directly. These 3 topics aren't a huge > >>>>> concern > >>>>>> for MM2 as they are a small set of topics and can be created > >> up-front > >>>> as > >>>>> a > >>>>>> one-time job. However, the main concern of the KIP is addressing > >> the > >>>>>> mirrored topics, synced configs, synced ACLs and the internal > >> topics > >>> of > >>>>> MM2 > >>>>>> which are needed for MM2 features like heartbeat and offset mapping > >>>>> between > >>>>>> Kafka clusters. > >>>>>> > >>>>>> The KIP states that Mirror Maker 2 will "Use > >>>>>>> ForwardingAdmin in MirrorUtils instead of TopicAdmin to create > >>>> internal > >>>>>>> compacted topics", but IIUC these topics (the ones created with > >> the > >>>>>>> MirrorUtils class) are Mirror Maker 2-specific and different from > >>> the > >>>>>>> Connect framework's internal topics. > >>>>>> > >>>>>> MM2 has been using 2 patterns to create topics > >>>>>> 1- Use AdminClient directly to create/update mirrored topics and > >>> their > >>>>> ACLs > >>>>>> 2- Use TopicAdmin in MirrorUtils to create MM2 internal topics > >> which > >>>> are > >>>>>> heartbeat, mm2-offset-syncs.<cluster_alias>.internal and > >>>>>> <cluster_alias>.checkpoints.internal > >>>>>> > >>>>>> This KIP will only replace AdminClient and TopicAdmin in the MM2 > >>>> codebase > >>>>>> by ForwardingAdmin and not connect related topics. > >>>>>> As Colin mentioned before we can have a feature KIP where we use > >>>>>> ForwardingAdmin outside MM2 but this is not addressed in this KIP. > >>>>>> > >>>>>> Hope this answered your questions. > >>>>>> > >>>>>> Best > >>>>>> Omnia > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Wed, Jun 1, 2022 at 2:14 AM Chris Egerton < > >>> fearthecel...@gmail.com> > >>>>>> wrote: > >>>>>> > >>>>>>> Hi Omnia, > >>>>>>> > >>>>>>> Thank you for your patience with this KIP! I have a few quick > >>>> thoughts: > >>>>>>> > >>>>>>> 1. Is the introduction of the new "ForwardingAdmin" class > >>> necessary, > >>>> or > >>>>>> can > >>>>>>> the same behavior can be achieved by subclassing the existing > >>>>>>> KafkaAdminClient class? > >>>>>>> > >>>>>>> 2. Would it be just as accurate to name the new Mirror Maker 2 > >>>> property > >>>>>>> "admin.class" instead of "forwarding.admin.class"? I think > >> brevity > >>>> may > >>>>>> work > >>>>>>> in our favor here > >>>>>>> > >>>>>>> 3. Would the admin class specified by the user also take effect > >> for > >>>>>> KIP-158 > >>>>>>> [1] style automatic topic creation? (Forgive me if this isn't > >>>>> applicable > >>>>>>> for Mirror Maker 2; I'm asking solely based on the knowledge that > >>> MM2 > >>>>> can > >>>>>>> be run as a source connector and has its own source task class > >>> [2].) > >>>>>>> > >>>>>>> 4. Would the admin class specified by the user also take effect > >> for > >>>>>>> internal topics created by the Connect framework (i.e., the > >> statue, > >>>>>> config, > >>>>>>> and offsets topics)? The KIP states that Mirror Maker 2 will "Use > >>>>>>> ForwardingAdmin in MirrorUtils instead of TopicAdmin to create > >>>> internal > >>>>>>> compacted topics", but IIUC these topics (the ones created with > >> the > >>>>>>> MirrorUtils class) are Mirror Maker 2-specific and different from > >>> the > >>>>>>> Connect framework's internal topics. > >>>>>>> > >>>>>>> [1] - > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-158%3A+Kafka+Connect+should+allow+source+connectors+to+set+topic-specific+settings+for+new+topics > >>>>>>> [2] - > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://github.com/apache/kafka/blob/4c9eeef5b2dff9a4f0977fbc5ac7eaaf930d0d0e/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceTask.java > >>>>>>> > >>>>>>> Cheers, > >>>>>>> > >>>>>>> Chris > >>>>>>> > >>>>>>> On Wed, May 25, 2022 at 5:26 AM Omnia Ibrahim < > >>>> o.g.h.ibra...@gmail.com > >>>>>> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi everyone, If there's no major concern anymore, I'll start > >> the > >>>>>>>> voting process. > >>>>>>>> > >>>>>>>> On Fri, May 20, 2022 at 5:58 PM Omnia Ibrahim < > >>>>> o.g.h.ibra...@gmail.com > >>>>>>> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Hi Colin, > >>>>>>>>> > >>>>>>>>>> Thanks for the clarification. I agree it's reasonable for > >>> people > >>>>> to > >>>>>>> want > >>>>>>>>> to use their own implementations of Admin. And we could have > >> a > >>>>> config > >>>>>>> for > >>>>>>>>> this, so that it becomes pluggable (possibly in other places > >>> than > >>>>>>>>> MirrorMaker, although we don't have to do that in this KIP). > >>>>>>>>>> > >>>>>>>>> Allowing people to plug custom implementation of Admin in > >> other > >>>>>> places > >>>>>>>>> sounds like a neat idea indeed. It can be nice addition for > >>>>> example ` > >>>>>>>>> org.apache.kafka.connect.util.SharedTopicAdmin` in Connect to > >>> use > >>>>>>> custom > >>>>>>>>> Admin as well. But agree no need to have it in this KIP. > >>>>>>>>>> We could even try to make this easier on developers. For > >>>> example, > >>>>> we > >>>>>>>>> could provide a public ForwardingAdmin class that forwards > >> all > >>>>>> requests > >>>>>>>> to > >>>>>>>>> the regular KafkaAdminClient. Then, people could make their > >>>> custom > >>>>>>> class > >>>>>>>>> inherit from ForwardingAdmin and override >just the specific > >>>>> methods > >>>>>>> that > >>>>>>>>> they wanted to override. So they don't have to implement all > >>> the > >>>>>>> methods, > >>>>>>>>> but just the ones that are different for them. > >>>>>>>>>> > >>>>>>>>>> I just wanted to make sure we weren't creating a second > >> Admin > >>>>> client > >>>>>>>>> interface -- I think that would really be hard for us to > >>> support > >>>>>>>> long-term. > >>>>>>>>> > >>>>>>>>> Forwarding would defiantly make it easier. I have updated the > >>> KIP > >>>>> to > >>>>>>>>> introduce ForwardingAdmin as well. > >>>>>>>>> > >>>>>>>>> regards, > >>>>>>>>> Omnia > >>>>>>>>> > >>>>>>>>> On Mon, May 16, 2022 at 9:31 PM Colin McCabe < > >>> cmcc...@apache.org > >>>>> > >>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> On Mon, May 16, 2022, at 10:24, Omnia Ibrahim wrote: > >>>>>>>>>>> Hi Colin, > >>>>>>>>>>> > >>>>>>>>>>> Thanks for your reply. > >>>>>>>>>>> > >>>>>>>>>>> This KIP doesn’t aim to solve any security concerns, but > >>>> rather > >>>>> a > >>>>>>>>>> conflict > >>>>>>>>>>> of responsibilities within any Kafka ecosystem that > >> includes > >>>> MM2 > >>>>>> and > >>>>>>>> any > >>>>>>>>>>> resource management solution. I’m not sure that was clear, > >>> so > >>>>> I’m > >>>>>>>>>> concerned > >>>>>>>>>>> about the motivation for your suggestion to close this > >> KIP. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Hi Omnia, > >>>>>>>>>> > >>>>>>>>>> Thanks for the clarification. I agree it's reasonable for > >>> people > >>>>> to > >>>>>>> want > >>>>>>>>>> to use their own implementations of Admin. And we could > >> have a > >>>>>> config > >>>>>>>> for > >>>>>>>>>> this, so that it becomes pluggable (possibly in other places > >>>> than > >>>>>>>>>> MirrorMaker, although we don't have to do that in this KIP). > >>>>>>>>>> > >>>>>>>>>> We could even try to make this easier on developers. For > >>>> example, > >>>>> we > >>>>>>>>>> could provide a public ForwardingAdmin class that forwards > >> all > >>>>>>> requests > >>>>>>>> to > >>>>>>>>>> the regular KafkaAdminClient. Then, people could make their > >>>> custom > >>>>>>> class > >>>>>>>>>> inherit from ForwardingAdmin and override just the specific > >>>>> methods > >>>>>>> that > >>>>>>>>>> they wanted to override. So they don't have to implement all > >>> the > >>>>>>>> methods, > >>>>>>>>>> but just the ones that are different for them. > >>>>>>>>>> > >>>>>>>>>> I just wanted to make sure we weren't creating a second > >> Admin > >>>>> client > >>>>>>>>>> interface -- I think that would really be hard for us to > >>> support > >>>>>>>> long-term. > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> It is generally accepted that resource management should > >> be > >>>>>>>> centralized, > >>>>>>>>>>> especially on the scale of mirroring N number of clusters. > >>> The > >>>>>> point > >>>>>>>> of > >>>>>>>>>>> this KIP is that any sort of topic management / federate > >>>>> solution > >>>>>> / > >>>>>>>>>>> up-front capacity planning system will be at odds with MM2 > >>> if > >>>>> MM2 > >>>>>>>> keeps > >>>>>>>>>>> using the Admin client directly. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Thanks for the explanation. That makes sense. > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> I understand your concern that the interface proposed in > >> the > >>>>> first > >>>>>>>>>> approach > >>>>>>>>>>> may become too similar to the existing Admin interface. > >> I’ll > >>>>>> update > >>>>>>>> the > >>>>>>>>>>> proposal by moving Ryanne’s previous suggestion to re-use > >>> the > >>>>>> Admin > >>>>>>>>>>> interface and add configuration to accept a custom > >>>>> implementation. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> +1. > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> If you still feel this KIP should be closed but can > >>> understand > >>>>> its > >>>>>>>>>>> motivation I can close this one and create a new one. > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I think it's reasonable to keep this one open and make the > >>>> changes > >>>>>> you > >>>>>>>>>> talked about above. > >>>>>>>>>> > >>>>>>>>>> regards, > >>>>>>>>>> Colin > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Thanks, > >>>>>>>>>>> Omnia > >>>>>>>>>>> > >>>>>>>>>>> On Fri, May 13, 2022 at 6:10 PM Colin McCabe < > >>>>> cmcc...@apache.org> > >>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> On Wed, May 11, 2022, at 15:07, Omnia Ibrahim wrote: > >>>>>>>>>>>>> Hi Colin, > >>>>>>>>>>>>> I don't mind the idea of MM2 users implementing the > >>>>> AdminClient > >>>>>>>>>>>> interface. > >>>>>>>>>>>>> However, there're two disadvantages to this. > >>>>>>>>>>>>> > >>>>>>>>>>>>> 1. Having around 70 methods definitions to have > >>>>>>> "NotImplemented" > >>>>>>>>>> is > >>>>>>>>>>>> one > >>>>>>>>>>>>> downside, and keep up with these if the AdminClient > >>>>>> interface > >>>>>>>>>> changes. > >>>>>>>>>>>>> 2. It makes it hard to list what admin functionality > >>> MM2 > >>>>>> uses > >>>>>>> as > >>>>>>>>>> MM2 > >>>>>>>>>>>>> interactions with AdminClient in the codebase are in > >>>> many > >>>>>>>> places. > >>>>>>>>>>>>> > >>>>>>>>>>>>> I guess it's OK for MM2 users who want to build their > >>> admin > >>>>>>> client > >>>>>>>> to > >>>>>>>>>>>> carry > >>>>>>>>>>>>> this burden, as I explained in my previous response to > >>> the > >>>>>>>> discussion > >>>>>>>>>>>>> thread. And we can do some cleanup to the codebase to > >>> have > >>>>> all > >>>>>>>> Admin > >>>>>>>>>>>>> interactions in MM2 in a utils class or something like > >>> that > >>>>> to > >>>>>>> make > >>>>>>>>>> it > >>>>>>>>>>>>> easier to navigate what MM2 needs from the Admin > >>> interface. > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Hi Omnia, > >>>>>>>>>>>> > >>>>>>>>>>>> Anyone who wants to extend Kafka with proprietary tooling > >>>> does > >>>>>> need > >>>>>>>> to > >>>>>>>>>>>> keep up with the Kafka API. We have done everything we > >> can > >>> to > >>>>>> make > >>>>>>>> this > >>>>>>>>>>>> easier. We rigorously define what the API is through the > >>> KIP > >>>>>>> process, > >>>>>>>>>> and > >>>>>>>>>>>> make it possible to extend by making it an interface > >> rather > >>>>> than > >>>>>>>>>> concrete > >>>>>>>>>>>> class. We also have a pretty lengthy deprecation process > >>> for > >>>>>> these > >>>>>>>>>> APIs. > >>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Maybe I'm misunderstanding the use-case you're > >> describing > >>>>> here. > >>>>>>> But > >>>>>>>>>> it > >>>>>>>>>>>>>> seems to me that if you create a proxy that has the > >>>> ability > >>>>> to > >>>>>>> do > >>>>>>>>>> any > >>>>>>>>>>>> admin > >>>>>>>>>>>>>> operation, and give MM2 access to that proxy, the > >>> security > >>>>>> model > >>>>>>>> is > >>>>>>>>>> the > >>>>>>>>>>>>>> same as just giving MM2 admin access. (Or it may be > >>> worse > >>>> if > >>>>>> the > >>>>>>>>>>>> sysadmin > >>>>>>>>>>>>>> doesn't know what this proxy is doing, and doesn't > >> lock > >>> it > >>>>>>>> down...) > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> MM2 runs with the assumption that it has > >>>>>>>>>>>>> > >>>>>>>>>>>>> - "CREATE" ACLs for topics on the source clusters to > >>>>> create > >>>>>>>>>>>> `heartbeat` > >>>>>>>>>>>>> topics. > >>>>>>>>>>>>> - "CREATE" and "ALTER" ACLs to create topics, add > >>>>>> partitions, > >>>>>>>>>> update > >>>>>>>>>>>>> topics' config and topics' ACLs (in future, will > >> also > >>>>>> include > >>>>>>>>>> group > >>>>>>>>>>>> ACLS as > >>>>>>>>>>>>> Mikael mentioned before in the thread) on the > >>>> destination > >>>>>>>>>> clusters. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Most organisations have some resource management or > >>>> federated > >>>>>>>>>> solutions > >>>>>>>>>>>>> (some would even have a budget system as part of these > >>>>> systems) > >>>>>>> to > >>>>>>>>>> manage > >>>>>>>>>>>>> Kafka resources, and these systems are usually the only > >>>>>>> application > >>>>>>>>>>>> allowed > >>>>>>>>>>>>> to initializing a client with "CREATE" and "ALTER" > >> ACLs. > >>>> They > >>>>>>> don't > >>>>>>>>>> grant > >>>>>>>>>>>>> these ACLs to any other teams/groups/applications to > >>> create > >>>>>> such > >>>>>>> a > >>>>>>>>>> client > >>>>>>>>>>>>> outside these systems, so assuming MM2 can bypass these > >>>>> systems > >>>>>>> and > >>>>>>>>>> use > >>>>>>>>>>>> the > >>>>>>>>>>>>> AdminClient directly to create/update resources isn't > >>>> valid. > >>>>>> This > >>>>>>>> is > >>>>>>>>>> the > >>>>>>>>>>>>> primary concern here. > >>>>>>>>>>>>> > >>>>>>>>>>>>> The KIP is trying to give MM2 more flexibility to allow > >>>>>>>>>> organisations to > >>>>>>>>>>>>> integrate MM2 with their resource management system as > >>> they > >>>>> see > >>>>>>> fit > >>>>>>>>>>>> without > >>>>>>>>>>>>> forcing them to disable most MM2 features. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Hope this make sense and clear it up. > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> The point I was trying to make is that there is no > >>> additional > >>>>>>>> security > >>>>>>>>>>>> here. If you have some agent that has all the > >> permissions, > >>>> and > >>>>>> MM2 > >>>>>>>> can > >>>>>>>>>> talk > >>>>>>>>>>>> to that agent and tell it what to do, then that is > >>> equivalent > >>>>> to > >>>>>>> just > >>>>>>>>>>>> giving MM2 all the permissions. So while there may be > >> other > >>>>>> reasons > >>>>>>>> to > >>>>>>>>>> use > >>>>>>>>>>>> this kind of agent-based architecture, added security > >> isn't > >>>>> one. > >>>>>>>>>>>> > >>>>>>>>>>>> In any case, I think we should close this KIP since we > >>>> already > >>>>>> have > >>>>>>>> an > >>>>>>>>>>>> Admin API. There isn't a need to create a public API for > >>>> admin > >>>>>>>>>> operations. > >>>>>>>>>>>> > >>>>>>>>>>>> best, > >>>>>>>>>>>> Colin > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Wed, May 11, 2022 at 9:09 PM Colin McCabe < > >>>>>> cmcc...@apache.org > >>>>>>>> > >>>>>>>>>> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>>> Hi Omnia Ibrahim, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I'm sorry, but I am -1 on adding competing Admin > >>>> interfaces. > >>>>>>> This > >>>>>>>>>> would > >>>>>>>>>>>>>> create confusion and a heavier maintenance burden for > >>> the > >>>>>>> project. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Since the org.apache.kafka.clients.admin.Admin > >> interface > >>>> is > >>>>> a > >>>>>>> Java > >>>>>>>>>>>>>> interface, any third-party software that wants to > >> insert > >>>> its > >>>>>> own > >>>>>>>>>>>>>> implementation of the interface can do so already. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> A KIP to make the Admin class used pluggable for MM2 > >>> would > >>>>> be > >>>>>>>>>>>> reasonable. > >>>>>>>>>>>>>> Adding a competing admin API is not. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> It's true that there are many Admin methods, but you > >> do > >>>> not > >>>>>> need > >>>>>>>> to > >>>>>>>>>>>>>> implement all of them -- just the ones that > >> MirrorMaker > >>>>> uses. > >>>>>>> The > >>>>>>>>>> other > >>>>>>>>>>>>>> ones can throw a NotImplementedException or similar. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> The current approach also assumes that the user > >>> running > >>>>> MM2 > >>>>>>> has > >>>>>>>>>> the > >>>>>>>>>>>>>> Admin right to > >>>>>>>>>>>>>>> create/update topics, which is only valid if the > >> user > >>>> who > >>>>>> runs > >>>>>>>> MM2 > >>>>>>>>>>>> also > >>>>>>>>>>>>>> manages both > >>>>>>>>>>>>>>> source and destination clusters. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Maybe I'm misunderstanding the use-case you're > >>> describing > >>>>>> here. > >>>>>>>> But > >>>>>>>>>> it > >>>>>>>>>>>>>> seems to me that if you create a proxy that has the > >>>> ability > >>>>> to > >>>>>>> do > >>>>>>>>>> any > >>>>>>>>>>>> admin > >>>>>>>>>>>>>> operation, and give MM2 access to that proxy, the > >>> security > >>>>>> model > >>>>>>>> is > >>>>>>>>>> the > >>>>>>>>>>>>>> same as just giving MM2 admin access. (Or it may be > >>> worse > >>>> if > >>>>>> the > >>>>>>>>>>>> sysadmin > >>>>>>>>>>>>>> doesn't know what this proxy is doing, and doesn't > >> lock > >>> it > >>>>>>>> down...) > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> best, > >>>>>>>>>>>>>> Colin > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On Mon, May 9, 2022, at 13:21, Omnia Ibrahim wrote: > >>>>>>>>>>>>>>> Hi, I gave the KIP another look after talking to > >> some > >>>>> people > >>>>>>> at > >>>>>>>>>> the > >>>>>>>>>>>> Kafka > >>>>>>>>>>>>>>> Summit in London. And I would like to clear up the > >>>>>> motivation > >>>>>>> of > >>>>>>>>>> this > >>>>>>>>>>>>>> KIP. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> At the moment, MM2 has some opinionated decisions > >> that > >>>> are > >>>>>>>>>> creating > >>>>>>>>>>>>>> issues > >>>>>>>>>>>>>>> for teams that use IaC, federated solutions or have > >> a > >>>>>>>>>> capacity/budget > >>>>>>>>>>>>>>> planning system for Kafka destination clusters. To > >>>> explain > >>>>>> it > >>>>>>>>>> better, > >>>>>>>>>>>>>> let's > >>>>>>>>>>>>>>> assume we have MM2 with the following configurations > >>> to > >>>>>>>> highlight > >>>>>>>>>>>> these > >>>>>>>>>>>>>>> problems. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> topics = .* > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> refresh.topics.enabled = true > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> sync.topic.configs.enabled = true > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> sync.topic.acls.enabled = true > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> // Maybe in futrue we can have > >>> sync.group.acls.enabled = > >>>>>> true > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> These configurations allow us to run MM2 with the > >>> value > >>>> of > >>>>>> its > >>>>>>>>>> full > >>>>>>>>>>>>>>> features. However, there are two main concerns when > >> we > >>>> run > >>>>>> on > >>>>>>> a > >>>>>>>>>> scale > >>>>>>>>>>>>>> with > >>>>>>>>>>>>>>> these configs: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1. *Capacity/Budgeting Planning:* > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Functionality or features that impact capacity > >>> planning > >>>>>> using > >>>>>>>> MM2 > >>>>>>>>>> are: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1. MM2 automatically creates topics (breaking the > >>>> rule > >>>>> of > >>>>>>>>>>>>>>> `auto.create.topics.enable=false`) and creates > >>> topic > >>>>>>>>>> partitions on > >>>>>>>>>>>>>>> destination clusters if the number of partitions > >>>>>> increases > >>>>>>> on > >>>>>>>>>> the > >>>>>>>>>>>>>> source. > >>>>>>>>>>>>>>> In the previous example, this functionality will > >>>> apply > >>>>> to > >>>>>>> any > >>>>>>>>>> topic > >>>>>>>>>>>>>> that > >>>>>>>>>>>>>>> matches the regex of the `topics` config. > >>>>>>>>>>>>>>> 2. Sync topic configs include configurations that > >>>>> impact > >>>>>>>>>> capacity > >>>>>>>>>>>>>> like ` > >>>>>>>>>>>>>>> retention.ms` and `retention.bytes`. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> These 2 points lead to adding new untracked capacity > >>> to > >>>>>>>>>> destination > >>>>>>>>>>>>>>> clusters without a way to count for them up-front or > >>>>>> safeguard > >>>>>>>> the > >>>>>>>>>>>>>> cluster. > >>>>>>>>>>>>>>> The team that runs the cluster will only see the > >>>> capacity > >>>>>>> issue > >>>>>>>>>> when > >>>>>>>>>>>>>> their > >>>>>>>>>>>>>>> disk usage hits the threshold for their alerts. The > >>> desk > >>>>>>>> capacity > >>>>>>>>>>>> issue > >>>>>>>>>>>>>> can > >>>>>>>>>>>>>>> be avoided if MM2 is flexible enough to > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> - have a way for teams that run their ecosystem > >> to > >>>> have > >>>>>> MM2 > >>>>>>>>>> behave > >>>>>>>>>>>>>>> within their system. > >>>>>>>>>>>>>>> - disable the auto-creation and avoid syncing > >>> configs > >>>>>> that > >>>>>>>>>> impact > >>>>>>>>>>>>>>> capacity > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 2. *Provisioning conflict:* > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> In the previous MM2 configurations; we ended up with > >>>>>> conflict > >>>>>>> as > >>>>>>>>>> MM2 > >>>>>>>>>>>> used > >>>>>>>>>>>>>>> `AdminClient` directly to perform the following > >>>>>> functionality > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> - Create a Kafka topic (no way to disable this > >> at > >>>> the > >>>>>>>> moment) > >>>>>>>>>>>>>>> - Add new Kafka partitions (no way to disable > >> this > >>>> at > >>>>>> the > >>>>>>>>>> moment) > >>>>>>>>>>>>>>> - Sync Kafka Topic configurations (can be > >>> disabled, > >>>>> but > >>>>>>> then > >>>>>>>>>> this > >>>>>>>>>>>>>>> reduces the value of MM2 potential for users) > >>>>>>>>>>>>>>> - Sync Kafka topic's ACLs (can be disabled, but > >>> this > >>>>>>> reduces > >>>>>>>>>> the > >>>>>>>>>>>>>> users' > >>>>>>>>>>>>>>> value). Disabling this feature also means that > >>> users > >>>>> must > >>>>>>>>>> ensure > >>>>>>>>>>>> they > >>>>>>>>>>>>>> have > >>>>>>>>>>>>>>> the right ACLs to the mirrored topics on the > >>>>> destination > >>>>>>>>>> cluster > >>>>>>>>>>>>>> before > >>>>>>>>>>>>>>> switching their consumers, especially when MM2 is > >>>> used > >>>>>> for > >>>>>>>>>> disaster > >>>>>>>>>>>>>>> recovery. It may lead to extra downtime for them. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> All these functionalities are using AdminClient; > >> which > >>>>>> causes > >>>>>>> an > >>>>>>>>>> issue > >>>>>>>>>>>>>> with > >>>>>>>>>>>>>>> teams that > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> - Manage their Kafka resources using tools like > >>>>> Strimizi > >>>>>> or > >>>>>>>>>> custom > >>>>>>>>>>>>>>> federated solutions. For example, Strimizi's > >>>>> UserOperator > >>>>>>>>>> doesn't > >>>>>>>>>>>>>> sync the > >>>>>>>>>>>>>>> topic ACLs when MM2 is enabled. Strimzi > >>> documentation > >>>>>>>> mentions > >>>>>>>>>> that > >>>>>>>>>>>>>> users > >>>>>>>>>>>>>>> must to disable MM2 `sync.topic.acls.enabled` if > >>> they > >>>>> use > >>>>>>>>>>>>>> `UserOperator`. > >>>>>>>>>>>>>>> On the other hand, Strimizi's TopicOperator > >> doesn't > >>>>> have > >>>>>>> the > >>>>>>>>>> same > >>>>>>>>>>>>>> issue > >>>>>>>>>>>>>>> because it has a bi-directional reconciliation > >>>> process > >>>>>> that > >>>>>>>>>> watches > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>> topics state on the Kafka cluster and updates > >>>>> KafkaTopic > >>>>>>>>>> resources > >>>>>>>>>>>> for > >>>>>>>>>>>>>>> Strimzi. This design works fine with Kafka MM2 > >> for > >>>>> Topics > >>>>>>> but > >>>>>>>>>> not > >>>>>>>>>>>> for > >>>>>>>>>>>>>>> syncing ACLs. Strimizi TopicOperator also doesn't > >>>> have > >>>>> a > >>>>>>> way > >>>>>>>> to > >>>>>>>>>>>> stop > >>>>>>>>>>>>>>> syncing config that impact capacity for example > >>>>> retention > >>>>>>>>>> configs. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> - Teams that run MM2 but don't own the > >> destination > >>>>>> cluster. > >>>>>>>> In > >>>>>>>>>> this > >>>>>>>>>>>>>>> case, these teams don't have Admin access, but > >> they > >>>> may > >>>>>>> have > >>>>>>>>>> Kafka > >>>>>>>>>>>>>>> management solutions, such as yahoo/CMAK or an > >>>> in-house > >>>>>>>>>> solution. > >>>>>>>>>>>> For > >>>>>>>>>>>>>> such > >>>>>>>>>>>>>>> a tool as CMAK, these teams can update/create > >>>> resources > >>>>>>> using > >>>>>>>>>> CMAK > >>>>>>>>>>>>>> REST API. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> The Proposed KIP gives users the flexibility to > >>>> integrate > >>>>>> MM2 > >>>>>>>>>> within > >>>>>>>>>>>>>> their > >>>>>>>>>>>>>>> ecosystem without disabling any MM2 features. We can > >>>>> achieve > >>>>>>>> this > >>>>>>>>>>>>>>> flexibility with one of the following solutions. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1. Introduce a new interface that hides Admin > >>>>>> interactions > >>>>>>> in > >>>>>>>>>> one > >>>>>>>>>>>>>> place. > >>>>>>>>>>>>>>> Then users can provide their way of resource > >>>>> management. > >>>>>> As > >>>>>>>>>> well as > >>>>>>>>>>>>>> clean > >>>>>>>>>>>>>>> up the MM2 code by having one place that manages > >>> the > >>>>>>>>>> resources, as > >>>>>>>>>>>> at > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>> moment, MM2 usage of AdminClient is all over the > >>>> code. > >>>>>>>>>>>>>>> 2. The second solution could be to add only a new > >>>>> config > >>>>>>> that > >>>>>>>>>>>> allows > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>> users to override AdminClient with another > >>>>> implementation > >>>>>>> of > >>>>>>>>>> the > >>>>>>>>>>>>>>> AdminClient interface, as Ryanne suggested > >> before. > >>>> The > >>>>>>>>>> downside is > >>>>>>>>>>>>>> that > >>>>>>>>>>>>>>> AdminClient is enormous and constantly under > >>>>> development, > >>>>>>> so > >>>>>>>>>> any > >>>>>>>>>>>>>> users who > >>>>>>>>>>>>>>> opt-in for custom implementation will need to > >> carry > >>>>> this > >>>>>>>>>> burden. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I favour the first solution as it will make it > >> either > >>>>> later > >>>>>> to > >>>>>>>>>> add any > >>>>>>>>>>>>>> new > >>>>>>>>>>>>>>> feature related to resource management. But don't > >> mind > >>>> if > >>>>>>> others > >>>>>>>>>> think > >>>>>>>>>>>>>> that > >>>>>>>>>>>>>>> the second solution is easier for MM2 design. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> *Note*: There are two possible future KIPs following > >>>> this > >>>>>> KIP > >>>>>>> to > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1. Add config to disable MM2 from auto creating > >> or > >>>>> adding > >>>>>>> new > >>>>>>>>>> topic > >>>>>>>>>>>>>>> partitions. > >>>>>>>>>>>>>>> 2. Add a way to exclude a specific topic's > >>>>> configuration > >>>>>>> from > >>>>>>>>>> being > >>>>>>>>>>>>>>> synced. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I hope this clears up the problem better. Please let > >>> me > >>>>> know > >>>>>>>> what > >>>>>>>>>> do > >>>>>>>>>>>> you > >>>>>>>>>>>>>>> think. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On Mon, Feb 7, 2022 at 3:26 PM Omnia Ibrahim < > >>>>>>>>>> o.g.h.ibra...@gmail.com > >>>>>>>>>>>>> > >>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Hi Mickael. Thanks for the feedback. I address some > >>> of > >>>>> your > >>>>>>>>>> points > >>>>>>>>>>>>>> below. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> *> This seems to address a relatively advanced and > >>>>> specific > >>>>>>> use > >>>>>>>>>> case* > >>>>>>>>>>>>>>>> The main point of the KIP is that MM2 is making a > >>>> massive > >>>>>>>>>> assumption > >>>>>>>>>>>>>> that > >>>>>>>>>>>>>>>> it has the right to alter/create resources. This > >>>>> assumption > >>>>>>>> isn't > >>>>>>>>>>>> valid > >>>>>>>>>>>>>> in > >>>>>>>>>>>>>>>> the world of Infra-as-Code, federated solutions and > >>>>>>> popularity > >>>>>>>>>> of OS > >>>>>>>>>>>>>> Kafka > >>>>>>>>>>>>>>>> Kubernetes operators; these infra/resource > >> management > >>>>>>> solutions > >>>>>>>>>>>> aren't > >>>>>>>>>>>>>>>> advanced use-cases anymore nowadays. These concerns > >>> had > >>>>>> been > >>>>>>>>>> raised > >>>>>>>>>>>> in > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>> past, especially regarding the assumption that MM2 > >>> can > >>>>>> create > >>>>>>>>>> topics > >>>>>>>>>>>> on > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>> destination cluster. For example, > >>>>>>>>>>>>>>>> https://issues.apache.org/jira/browse/KAFKA-12753 > >>> and > >>>>>>>>>>>>>>>> > >>>>>>>> > >> https://www.mail-archive.com/dev@kafka.apache.org/msg119340.html > >>>>>>>>>> . > >>>>>>>>>>>>>>>> The primary motivation is giving some power to data > >>>>>>>>>>>>>>>> platform/infrastructure team to make MM2 part of > >>> their > >>>>>>> internal > >>>>>>>>>> Kafka > >>>>>>>>>>>>>>>> ecosystem without dropping the main features that > >>> make > >>>>> MM2 > >>>>>>>>>> valuable, > >>>>>>>>>>>>>> like > >>>>>>>>>>>>>>>> syncing topic configs. For example, if someone uses > >>> any > >>>>> OS > >>>>>>>> Kafka > >>>>>>>>>> k8s > >>>>>>>>>>>>>>>> operator, they can implement the class to interact > >>> with > >>>>> the > >>>>>>> k8s > >>>>>>>>>>>>>> operator to > >>>>>>>>>>>>>>>> create these resources. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> *> My initial concern is this may make it hard to > >>>> evolve > >>>>>>>>>> MirrorMaker > >>>>>>>>>>>> as > >>>>>>>>>>>>>>>> we'll likely need to update this new interface if > >> new > >>>>>>> features > >>>>>>>>>> are > >>>>>>>>>>>>>> added.* > >>>>>>>>>>>>>>>> I agree it's a disadvantage to adding a new > >> interface > >>>>>> however > >>>>>>>>>> adding > >>>>>>>>>>>>>> more > >>>>>>>>>>>>>>>> admin interactions from MM2 to alter/create > >> resources > >>>> and > >>>>>>>> access > >>>>>>>>>> will > >>>>>>>>>>>>>> feed > >>>>>>>>>>>>>>>> the main issue as I mentioned above with the > >>> popularity > >>>>> of > >>>>>>> IaC > >>>>>>>>>> and > >>>>>>>>>>>>>>>> federated solutions; most data > >>> platform/infrastructure > >>>>>> teams > >>>>>>>> will > >>>>>>>>>>>> endup > >>>>>>>>>>>>>>>> disabling these new features. > >>>>>>>>>>>>>>>> Also, at the moment, most MM2 interactions with the > >>>> admin > >>>>>>>> client > >>>>>>>>>> are > >>>>>>>>>>>>>>>> scattered across the codebase so having one place > >>> where > >>>>> all > >>>>>>>> admin > >>>>>>>>>>>>>>>> interactions are listed isn't a bad thing. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> *> For example if we wanted to sync group ACLs.* > >>>>>>>>>>>>>>>> As I mentioned before, altering any resource's > >>>>>> configurations > >>>>>>>>>> with > >>>>>>>>>>>> MM2 > >>>>>>>>>>>>>> is > >>>>>>>>>>>>>>>> the one main concern for any data > >>>> platform/infrastructure > >>>>>>> team > >>>>>>>>>> that > >>>>>>>>>>>>>> wants > >>>>>>>>>>>>>>>> to have control over their clusters and use MM2. So > >>> the > >>>>>> main > >>>>>>>>>> question > >>>>>>>>>>>>>> with > >>>>>>>>>>>>>>>> adding any new altering feature like sync group > >> ACLs > >>>> will > >>>>>>> raise > >>>>>>>>>> the > >>>>>>>>>>>> same > >>>>>>>>>>>>>>>> question of how many teams will actually enable > >> this > >>>>>> feature. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> *>Regarding the proposed API, I have a few > >>> suggestions: > >>>>>> - > >>>>>>> What > >>>>>>>>>> about > >>>>>>>>>>>>>>>> using configure() instead of the constructor to > >> pass > >>>> the > >>>>>>>>>>>>> configuration, > >>>>>>>>>>>>>>>> especially as it's implementing Configurable >- > >> It's > >>>> not > >>>>>>> clear > >>>>>>>>>> what > >>>>>>>>>>>> all > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>> arguments of createTopicPartitions()>are. What's > >> the > >>>>>>> difference > >>>>>>>>>>>> between > >>>>>>>>>>>>>>>> partitionCounts and newPartitions?>Should we have > >>>>> separate > >>>>>>>>>> methods > >>>>>>>>>>>> for > >>>>>>>>>>>>>>>> creating topics and partitions? >- Do we really > >> need > >>>>>>>>>>>>>>>> createCompactedTopic()? >- Instead of > >>>>> updateTopicConfigs() > >>>>>>> and > >>>>>>>>>>>>>> updateAcls() > >>>>>>>>>>>>>>>> should we use the >"alter" prefix to stay > >> consistent > >>>> with > >>>>>>>> Admin?* > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> These are good suggestions that will update the KIP > >>> to > >>>>>>> address > >>>>>>>>>> these. > >>>>>>>>>>>>>>>> Regarding the `createCompactedTopic` MM2 is using > >>> this > >>>>>> method > >>>>>>>> to > >>>>>>>>>>>> create > >>>>>>>>>>>>>>>> internal topics. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thanks > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On Wed, Jan 26, 2022 at 1:55 PM Mickael Maison < > >>>>>>>>>>>>>> mickael.mai...@gmail.com> > >>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Hi Omnia, > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thanks for the KIP, sorry for taking so long to > >>>> comment. > >>>>>>> I've > >>>>>>>>>> only > >>>>>>>>>>>> had > >>>>>>>>>>>>>>>>> time to take a quick look so far. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> This seems to address a relatively advanced and > >>>> specific > >>>>>> use > >>>>>>>>>> case. > >>>>>>>>>>>> My > >>>>>>>>>>>>>>>>> initial concern is this may make it hard to evolve > >>>>>>> MirrorMaker > >>>>>>>>>> as > >>>>>>>>>>>>>>>>> we'll likely need to update this new interface if > >>> new > >>>>>>> features > >>>>>>>>>> are > >>>>>>>>>>>>>>>>> added. For example if we wanted to sync group > >> ACLs. > >>>>>>>>>>>>>>>>> I'm wondering if it's something you've thought > >>> about. > >>>>> I'm > >>>>>>> not > >>>>>>>>>> saying > >>>>>>>>>>>>>>>>> it's a blocker but we have to weigh the pros and > >>> cons > >>>>> when > >>>>>>>>>>>> introducing > >>>>>>>>>>>>>>>>> new features. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Regarding the proposed API, I have a few > >>> suggestions: > >>>>>>>>>>>>>>>>> - What about using configure() instead of the > >>>>> constructor > >>>>>> to > >>>>>>>>>> pass > >>>>>>>>>>>> the > >>>>>>>>>>>>>>>>> configuration, especially as it's implementing > >>>>>> Configurable > >>>>>>>>>>>>>>>>> - It's not clear what all the arguments of > >>>>>>>>>> createTopicPartitions() > >>>>>>>>>>>>>>>>> are. What's the difference between partitionCounts > >>> and > >>>>>>>>>>>> newPartitions? > >>>>>>>>>>>>>>>>> Should we have separate methods for creating > >> topics > >>>> and > >>>>>>>>>> partitions? > >>>>>>>>>>>>>>>>> - Do we really need createCompactedTopic()? > >>>>>>>>>>>>>>>>> - Instead of updateTopicConfigs() and updateAcls() > >>>>> should > >>>>>> we > >>>>>>>>>> use the > >>>>>>>>>>>>>>>>> "alter" prefix to stay consistent with Admin? > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>>> Mickael > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> On Wed, Jan 26, 2022 at 11:26 AM Omnia Ibrahim < > >>>>>>>>>>>>>> o.g.h.ibra...@gmail.com> > >>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Hi, > >>>>>>>>>>>>>>>>>> If there are no more concerns regarding the > >>> proposal > >>>>>> can I > >>>>>>>> get > >>>>>>>>>>>> some > >>>>>>>>>>>>>>>>> votes on the KIP here > >>>>>>>>>>>>>>>>> > >>>>>>>>>> > >>>> https://lists.apache.org/thread/950lpxjb5kbjm8qdszlpxm9h4j4sfyjx > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Thanks > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> On Wed, Oct 27, 2021 at 3:54 PM Ryanne Dolan < > >>>>>>>>>>>> ryannedo...@gmail.com> > >>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Well I'm convinced! Thanks for looking into it. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Ryanne > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> On Wed, Oct 27, 2021, 8:49 AM Omnia Ibrahim < > >>>>>>>>>>>>>> o.g.h.ibra...@gmail.com> > >>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> I checked the difference between the number > >> of > >>>>>> methods > >>>>>>> in > >>>>>>>>>> the > >>>>>>>>>>>>>> Admin > >>>>>>>>>>>>>>>>>>>> interface and the number of methods MM2 > >> invokes > >>>>> from > >>>>>>>>>> Admin, and > >>>>>>>>>>>>>> this > >>>>>>>>>>>>>>>>> diff > >>>>>>>>>>>>>>>>>>>> is enormous (it's more than 70 methods). > >>>>>>>>>>>>>>>>>>>> As far as I can see, the following methods > >> MM2 > >>>>>> depends > >>>>>>> on > >>>>>>>>>> (in > >>>>>>>>>>>>>>>>>>>> MirrorSourceConnector, MirrorMaker, > >>>>>>> MirrorCheckpointTask > >>>>>>>>>> and > >>>>>>>>>>>>>>>>>>>> MirrorCheckpointConnector), this will leave > >> 73 > >>>>>> methods > >>>>>>> on > >>>>>>>>>> the > >>>>>>>>>>>>>> Admin > >>>>>>>>>>>>>>>>>>>> interface that customer will need to return > >>> dummy > >>>>>> data > >>>>>>>> for, > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> 1. create(conf) > >>>>>>>>>>>>>>>>>>>> 2. close > >>>>>>>>>>>>>>>>>>>> 3. listTopics() > >>>>>>>>>>>>>>>>>>>> 4. createTopics(newTopics, > >>>> createTopicsOptions) > >>>>>>>>>>>>>>>>>>>> 5. createPartitions(newPartitions) > >>>>>>>>>>>>>>>>>>>> 6. alterConfigs(configs) // this method > >> is > >>>>> marked > >>>>>>> for > >>>>>>>>>>>>>>>>> deprecation in > >>>>>>>>>>>>>>>>>>>> Admin and the ConfigResource MM2 use is > >> only > >>>>> TOPIC > >>>>>>>>>>>>>>>>>>>> 7. createAcls(aclBindings) // the list of > >>>>> bindings > >>>>>>>>>> always > >>>>>>>>>>>>>>>>> filtered by > >>>>>>>>>>>>>>>>>>>> TOPIC > >>>>>>>>>>>>>>>>>>>> 8. describeAcls(aclBindingFilter) // > >> filter > >>> is > >>>>>>> always > >>>>>>>>>>>>>>>>> ANY_TOPIC_ACL > >>>>>>>>>>>>>>>>>>>> 9. describeConfigs(configResources) // > >>> Always > >>>>> for > >>>>>>>> TOPIC > >>>>>>>>>>>>>> resources > >>>>>>>>>>>>>>>>>>>> 10. listConsumerGroupOffsets(groupId) > >>>>>>>>>>>>>>>>>>>> 11. listConsumerGroups() > >>>>>>>>>>>>>>>>>>>> 12. alterConsumerGroupOffsets(groupId, > >>>> offsets) > >>>>>>>>>>>>>>>>>>>> 13. describeCluster() // this is invoked > >>> from > >>>>>>>>>>>>>>>>>>>> ConnectUtils.lookupKafkaClusterId(conf), > >>>>>>>>>>>>>>>>>>>> but MM2 isn't the one that initialize the > >>>>>>> AdminClient > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Going with the Admin interface in practice > >> will > >>>>> make > >>>>>>> any > >>>>>>>>>> custom > >>>>>>>>>>>>>> Admin > >>>>>>>>>>>>>>>>>>>> implementation humongous even for a fringe > >> use > >>>> case > >>>>>>>>>> because of > >>>>>>>>>>>> the > >>>>>>>>>>>>>>>>> number > >>>>>>>>>>>>>>>>>>>> of methods that need to return dummy data, > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> I am still leaning toward a new interface as > >> it > >>>>>>> abstract > >>>>>>>>>> all > >>>>>>>>>>>> MM2's > >>>>>>>>>>>>>>>>>>>> interaction with Kafka Resources in one > >> place; > >>>> this > >>>>>>> makes > >>>>>>>>>> it > >>>>>>>>>>>>>> easier > >>>>>>>>>>>>>>>>> to > >>>>>>>>>>>>>>>>>>>> maintain and make it easier for the use cases > >>>> where > >>>>>>>>>> customers > >>>>>>>>>>>>>> wish to > >>>>>>>>>>>>>>>>>>>> provide a different method to handle > >> resources. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Omnia > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> On Tue, Oct 26, 2021 at 4:10 PM Ryanne Dolan > >> < > >>>>>>>>>>>>>> ryannedo...@gmail.com> > >>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> I like the idea of failing-fast whenever a > >>>> custom > >>>>>>> impl > >>>>>>>> is > >>>>>>>>>>>>>>>>> provided, but I > >>>>>>>>>>>>>>>>>>>>> suppose that that could be done for Admin > >> as > >>>>> well. > >>>>>> I > >>>>>>>>>> agree > >>>>>>>>>>>> your > >>>>>>>>>>>>>>>>> proposal > >>>>>>>>>>>>>>>>>>>> is > >>>>>>>>>>>>>>>>>>>>> more ergonomic, but maybe it's okay to > >> have a > >>>>>> little > >>>>>>>>>>>> friction in > >>>>>>>>>>>>>>>>> such > >>>>>>>>>>>>>>>>>>>>> fringe use-cases. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> Ryanne > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> On Tue, Oct 26, 2021, 6:23 AM Omnia > >> Ibrahim < > >>>>>>>>>>>>>>>>> o.g.h.ibra...@gmail.com> > >>>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> Hey Ryanne, Thanks fo the quick feedback. > >>>>>>>>>>>>>>>>>>>>>> Using the Admin interface would make > >>>> everything > >>>>>>>>>> easier, as > >>>>>>>>>>>> MM2 > >>>>>>>>>>>>>>>>> will > >>>>>>>>>>>>>>>>>>>> need > >>>>>>>>>>>>>>>>>>>>>> only to configure the classpath for the > >> new > >>>>>>>>>> implementation > >>>>>>>>>>>> and > >>>>>>>>>>>>>>>>> use it > >>>>>>>>>>>>>>>>>>>>>> instead of AdminClient. > >>>>>>>>>>>>>>>>>>>>>> However, I have two concerns > >>>>>>>>>>>>>>>>>>>>>> 1. The Admin interface is enormous, and > >> the > >>>> MM2 > >>>>>>> users > >>>>>>>>>> will > >>>>>>>>>>>>>> need > >>>>>>>>>>>>>>>>> to know > >>>>>>>>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>>>>>> list of methods MM2 depends on and > >> override > >>>>> these > >>>>>>>> only > >>>>>>>>>>>>>> instead of > >>>>>>>>>>>>>>>>>>>>>> implementing the whole Admin interface. > >>>>>>>>>>>>>>>>>>>>>> 2. MM2 users will need keep an eye on any > >>>>> changes > >>>>>>> to > >>>>>>>>>> Admin > >>>>>>>>>>>>>>>>> interface > >>>>>>>>>>>>>>>>>>>> that > >>>>>>>>>>>>>>>>>>>>>> impact MM2 for example deprecating > >> methods. > >>>>>>>>>>>>>>>>>>>>>> Am not sure if adding these concerns on > >> the > >>>>> users > >>>>>>> is > >>>>>>>>>>>>>> acceptable > >>>>>>>>>>>>>>>>> or not. > >>>>>>>>>>>>>>>>>>>>>> One solution to address these concerns > >>> could > >>>> be > >>>>>>>> adding > >>>>>>>>>> some > >>>>>>>>>>>>>>>>> checks to > >>>>>>>>>>>>>>>>>>>>> make > >>>>>>>>>>>>>>>>>>>>>> sure the methods MM2 uses from the Admin > >>>>>> interface > >>>>>>>>>> exists > >>>>>>>>>>>> to > >>>>>>>>>>>>>> fail > >>>>>>>>>>>>>>>>>>>> faster. > >>>>>>>>>>>>>>>>>>>>>> What do you think > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> Omnia > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 25, 2021 at 11:24 PM Ryanne > >>>> Dolan < > >>>>>>>>>>>>>>>>> ryannedo...@gmail.com> > >>>>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> Thanks Omnia, neat idea. I wonder if we > >>>> could > >>>>>> use > >>>>>>>> the > >>>>>>>>>>>>>> existing > >>>>>>>>>>>>>>>>> Admin > >>>>>>>>>>>>>>>>>>>>>>> interface instead of defining a new > >> one? > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> Ryanne > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 25, 2021, 12:54 PM Omnia > >>>> Ibrahim > >>>>> < > >>>>>>>>>>>>>>>>>>>> o.g.h.ibra...@gmail.com> > >>>>>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>> Hey everyone, > >>>>>>>>>>>>>>>>>>>>>>>> Please take a look at KIP-787 > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-787%3A+MM2+Interface+to+manage+Kafka+resources > >>>>>>>>>>>>>>>>>>>>>>>> < > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-787%3A+MM2+Interface+to+manage+Kafka+resources > >>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>> Thanks for the feedback and support > >>>>>>>>>>>>>>>>>>>>>>>> Omnia > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >
