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