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

2024-03-27 Thread Greg Harris
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

2024-03-27 Thread Omnia Ibrahim
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

2024-03-14 Thread Greg Harris
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

2024-03-14 Thread Mickael Maison
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

2024-03-05 Thread Omnia Ibrahim
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