Hi Omnia! Thank you for your response. I want to make sure that we both understand the motivations and designs here, and we can make a fair evaluation.
> pluggable extension point Can you explain the difference? To me, both "plugin" and "pluggable extension point" refer to an API that lets downstream projects define custom code to execute within the Connect framework. > Am not sure I am getting your point here. I think consistency is important in API design, so that users can take concepts from one area and apply them again later. For example, a user that is familiar with implementing ConnectRestExtensions may learn that they can load arbitrary versions of libraries that the framework also uses. They then can implement and release a single artifact that works across all Connect versions. If that user then took that knowledge to the non-isolated ForwardingAdmin interface, they will have problems. Without classloader isolation to protect them from version conflicts, they will then need to distribute multiple artifacts that are compatible with each version of the framework, or only support a single version at a time. > This is a possibility, however if this is a limitation of KIP-787 then this > exists already when we run on connect cluster. Yes, and that is a limitation that I feel must be addressed when bringing this to the broader Connect framework. Connect goes to great lengths to allow users to compile against one version of the framework and run against another, in order to reduce the maintenance burden on plugin developers. The ForwardingAdmin being strongly coupled to the MM2 version is consistent with the other plugins within MM2 which are not isolated, but this is an exception in the broader Connect ecosystem, not the standard. > adding competing Admin interfaces would create confusion and a heavier > maintenance burden for the project. I am not suggesting a new Admin interface. When a new method is added to the Admin interface, Admin developers are burdened to choose an implementation for the ForwardingAdmin. Their implementation could either delegate to the real Admin instance, or throw an UnsupportedOperationException. This has already happened [1]. If they delegate to the underlying admin, the developer of the custom ForwardingAdmin subclass is not notified of the new operation after an upgrade, and the runtime could be "going around" the custom implementation for a long time without anyone noticing. If they throw an UnsupportedOperationException, the real Admin instance is inaccessible to the subclasses, as it is a private variable. Related to this, the ForwardingAdmin can't be used to enforce its own usage, and so is useless for security-oriented implementations. Anyone can configure the default ForwardingAdmin and get the raw AdminClient behavior. I understand that this is outside of its original use-case, but I think it's a natural property that would be desirable for a federation layer, but isn't possible with the current design. > There's no space in the Admin API to represent the cluster, and there > shouldn't be. I disagree with this statement. There are at least three ways to represent the cluster via the current Admin API, each with tradeoffs. 1. You can identify a client by credential. If you are generating a credential to give the Admin client, you can also remember what cluster that credential corresponds to. 2. You can identify a client by client.id, and parse/decode/lookup the cluster from the id. 3. You can identify a client by the endpoint they contact. If you give each client a unique endpoint, you can infer the client from which endpoint they contact. > Some of the requests involve discovering metadata. e.g. listing topics > involves discovering metadata and selecting the least busy node before > sending the RPC. Since your federation layer is in the middle of the connection, it has full control over all requests and responses. It can implement arbitrary behavior for the metadata operations, such as lying to the client about the identity of brokers and load. > 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. > ... this will add another network latency on these operators which are used > widely Yes this is correct, but that is the case regardless of which protocol is used between Connect and the federation layer. Your architecture may need to change slightly if you were relying heavily on the underlying AdminClient. > The Admin API uses a binary protocol, fit for interfacing with Kafka, whereas > a federation layer could use a simpler REST&JSON based interface I understand that implementing the binary Kafka protocol is a burden, but the extra effort is paid back in a much better composability and interoperability. You can also strategically re-use code to make implementing the API easier: Either pull the API definitions from AK, use the AK code generation, or use another project which re-implements the Kafka API. You may consider looking into Kroxylicious [2], other proxies, or other re-implementations of the Kafka protocol to see how they do it. > not all Java APIs ... only “MM2” and Connect ... only Stream By "one-by-one updating the Java APIs" I was not referring to Consumer and Producer. I understand that you don't intend to add pluggable interfaces for those. I was referring to the call-sites for the Java AdminClient, which you listed: MM2, Connect, Streams, plus Tools. From my perspective, these call-sites differ in _degree_ but not in _kind_. You appear to be solving these call-sites one-by-one as adoption or operational burden makes it necessary, when they are really all the same problem that require the same solution. Since the AdminClient is a public API, there are potentially many other call-sites in third-party code that use the AdminClient to auto-create topics in a similar fashion to MM2, Connect, and Streams. For example in the Connect space, the Debezium connectors [3] use AdminClient to create topics. It appears that the Debezium project would need to be intentionally onboarded to ForwardingAdmin to be usable within your infrastructure, which is a burden on the Debezium project who could otherwise be unaware of your federation layer entirely. What if you needed to interact with an AdminClient implementation in another programming language, or onboard a project you didn't have source access to? > what would your suggestion means for this KIP which was voted and approved > long time ago? I would expect to eventually deprecate ForwardingAdmin and remove it after a reasonable deprecation period, Kafka 5.0 at the earliest. Thanks, Greg [1] https://github.com/apache/kafka/pull/14811/files#diff-98a62720bf862a065ffc14dbc9437f913893a03bd46d6642e50dc98d2fc0e069R281 [2] https://kroxylicious.io/ [3] https://github.com/debezium/debezium/blob/9bc20c9ea3510e65f3f521a5dba5769a893a7ed0/debezium-storage/debezium-storage-kafka/src/main/java/io/debezium/storage/kafka/history/KafkaSchemaHistory.java#L420 On Wed, Mar 27, 2024 at 7:03 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com> wrote: > > 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 >