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