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