Hi Greg thanks for the feedback and sorry for the late response. > I don't think we can adapt the ForwardingAdmin as-is for use as a > first-class Connect plugin. > 1. It doesn't have a default constructor, and so can't be included in > the existing plugin discovery mechanisms. > 2. It doesn't implement Versioned, and so won't have version > information exposed in the REST API The goal isn't for ForwardingAdmin to become a Connect plugin but rather to introduce a pluggable extension point that intercepts or replaces Admin Client functionality. Especially since as far as I know Admin Client in Connect isn’t a plugin or any of Kafka clients. Maybe the KIP wasn’t clear enough on this front so I’ll update the KIP to make this clearer. Even if we want to be versioned we can have another KIP to do this but is there a reason why we'd want that?
> I also don't think that we should make the ForwardingAdmin a > second-class Connect plugin. > 1. Having some plugins but not others benefit from classloader > isolation would be a "gotcha" for anyone familiar with existing > Connect plugins Am not sure I am getting your point here. > 2. Some future implementations may have a use-case for classloader > isolation (such as depending on their own HTTP/json library) and > retrofitting isolation would be more complicated than including it > initially. This is a possibility, however if this is a limitation of KIP-787 then this exists already when we run on connect cluster. > I also have concerns about the complexity of the implementation as a > superclass instead of an interface, especially when considering the > evolution of the Admin interface. This was discussed in the alternatives and discussion thread for KIP-787 which is the original KIP. Having a delegator class like forwarding admin has some advantages over an interface or inheriting Kafka admin client having a delegator class over an interface or inheriting KafkaAdminClient would make it easier to test the customised implementation adding competing Admin interfaces would create confusion and a heavier maintenance burden for the project. Kafka already has this pattern of wrapper/delegator class for Admin client like `org.apache.kafka.streams.processor.internals.InternalTopicManager`,`org.apache.kafka.connect.util.SharedTopicAdmin` and `org.apache.kafka.connect.util.TopicAdmin` and never had another interface for AdminClient. > I don't think the original proposal included the rejected alternative > of having the existing AdminClient talk to the federation layer, which > could implement a Kafka-compatible endpoint. Thanks for suggesting that, indeed I had not included that option in the rejected alternatives. I think this approach should be ruled out for the following reasons (I will update the KIP to reflect this) The Admin API is an interface is modelled around a cluster, but the federation layer will have to encompass multiple ones, it operates at a different abstraction level, creating all sorts of problems such as – which cluster does request this refer too? There's no space in the Admin API to represent the cluster, and there shouldn't be. With the forwarding admin implementation, the cluster identifier can be configured locally and used accordingly. The Admin API expects Kafka a cluster to be configured as an endpoint. Some of the requests involve discovering metadata. e.g. listing topics involves discovering metadata and selecting the least busy node before sending the RPC. A federation layer should be in the same scope as any of this, it can be a stateless service. If the federation layer has any issue, this will block everyone use AdminClient for basic day-to-day functionality like create/alter resources creating a bottle neck. It also might block the operators of Kafka cluster as well (We can argue that the admin client can run in 2 modes with a flag one for enable federation layer and another without but still it is a blocker). This suggestion might be tricky with Kafka as a Services providers; who will provide this AdminClient implementation with federation layer? How this will work with K8S operators and IaC management tools? Specially when the operator is deployed in another K8S cluster this will add another network latency on these operators which are used widely The Admin API uses a binary protocol, fit for interfacing with Kafka, whereas a federation layer could use a simpler REST&JSON based interface > If a federation layer needs to intercept the Admin client behaviors, > It sounds more reasonable for that to be addressed for all Admin > clients at the network boundary rather than one-by-one updating the > Java APIs to use this new plugin. I want just to clarify not all Java APIs need this new plugin only “MM2” and Connect to make the deployment of running MM2 on connect fully managed with federation layer. The other API that might need this is only Stream for the initialising `InternalTopicManager` which create internal topics for the topology. These 3 APIs are bypassing disabling “auto.create.topics.enable” by using admin client which for people who run Kafka ecosystem this doesn’t always make sense as most of the time they disable “auto.create.topics.enable” because they have some sort of federation/provision/capacity planning layer to control the clusters. Clients like consumer/producer never needed AdminClient to create/alter their resources and as far as I know no one requested this feature in the community. And I don’t see the need for these clients to have it. It’s mostly frameworks that need these type of power. > This KIP appearing as a follow-up to KIP-787 is evidence that the > problem is more general than the proposed solution. As KIP-787 is built based on the idea that having a class delegate resource management to AdminClient isn’t new to Kafka what would your suggestion means for this KIP which was voted and approved long time ago? > At this time I'm -1 for this proposal. I'm happy to discuss this more > in the DISCUSS thread. I would keep it here as this will help others to decide how to vote forward. Thanks Omnia > On 14 Mar 2024, at 19:36, Greg Harris <greg.har...@aiven.io.INVALID> wrote: > > Hi Omnia, > > Thanks for the KIP. > > I don't think we can adapt the ForwardingAdmin as-is for use as a > first-class Connect plugin. > 1. It doesn't have a default constructor, and so can't be included in > the existing plugin discovery mechanisms. > 2. It doesn't implement Versioned, and so won't have version > information exposed in the REST API > > I also don't think that we should make the ForwardingAdmin a > second-class Connect plugin. > 1. Having some plugins but not others benefit from classloader > isolation would be a "gotcha" for anyone familiar with existing > Connect plugins > 2. Some future implementations may have a use-case for classloader > isolation (such as depending on their own HTTP/json library) and > retrofitting isolation would be more complicated than including it > initially. > > I also have concerns about the complexity of the implementation as a > superclass instead of an interface, especially when considering the > evolution of the Admin interface. > > I don't think the original proposal included the rejected alternative > of having the existing AdminClient talk to the federation layer, which > could implement a Kafka-compatible endpoint. > If a federation layer needs to intercept the Admin client behaviors, > It sounds more reasonable for that to be addressed for all Admin > clients at the network boundary rather than one-by-one updating the > Java APIs to use this new plugin. > This KIP appearing as a follow-up to KIP-787 is evidence that the > problem is more general than the proposed solution. > > At this time I'm -1 for this proposal. I'm happy to discuss this more > in the DISCUSS thread. > > Thanks, > Greg > > On Thu, Mar 14, 2024 at 11:07 AM Mickael Maison > <mickael.mai...@gmail.com> wrote: >> >> Hi Omnia, >> >> +1 (binding), thanks for the KIP >> >> Mickael >> >> On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com> >> wrote: >>> >>> Hi everyone, I would like to start the vote on KIP-981: Manage Connect >>> topics with custom implementation of Admin >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-981%3A+Manage+Connect+topics+with+custom+implementation+of+Admin >>> >>> Thanks >>> Omnia
