Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin
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, u
Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin
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` wh
Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin
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 wrote: > > Hi Omnia, > > +1 (binding), thanks for the KIP > > Mickael > > On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim 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
Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin
Hi Omnia, +1 (binding), thanks for the KIP Mickael On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim 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
[VOTE] KIP-981: Manage Connect topics with custom implementation of Admin
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