[VOTE] KIP-981: Manage Connect topics with custom implementation of Admin

[Omnia Ibrahim <o....@gmail.com>]

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

[Greg Harris <gr...@aiven.io.INVALID>]

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....@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 <gr...@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
> > <mi...@gmail.com> wrote:
> >>
> >> Hi Omnia,
> >>
> >> +1 (binding), thanks for the KIP
> >>
> >> Mickael
> >>
> >> On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim <o....@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
>

Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin

[Omnia Ibrahim <o....@gmail.com>]

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 <gr...@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
> <mi...@gmail.com> wrote:
>> 
>> Hi Omnia,
>> 
>> +1 (binding), thanks for the KIP
>> 
>> Mickael
>> 
>> On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim <o....@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

Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin

[Greg Harris <gr...@aiven.io.INVALID>]

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
<mi...@gmail.com> wrote:
>
> Hi Omnia,
>
> +1 (binding), thanks for the KIP
>
> Mickael
>
> On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim <o....@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

Re: [VOTE] KIP-981: Manage Connect topics with custom implementation of Admin

[Mickael Maison <mi...@gmail.com>]

Hi Omnia,

+1 (binding), thanks for the KIP

Mickael

On Tue, Mar 5, 2024 at 10:46 AM Omnia Ibrahim <o....@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