[fix #9640] remove pulsar-client-admin-api dependency : pulsar-common

[freeznet]

Fixes #9640
Part 3

Motivation

Module pulsar-client-admin-api has been introduced in #9246, but it pulling in many dependencies. 

This PR removes pulsar-common from the dependency, and resolves the issues.

With this PR, some temp dependencies like gson [#10513], swagger-annotations, slf4j-api, and commons-lang3 has been added. Later I will remove gson and commons-lang3, but I am not very sure about swagger-annotations and slf4j-api.

Modifications

remove pulsar-common from pulsar-client-admin-api.

Verifying this change

• Make sure that the change passes the CI checks.

[eolivelli]

I am fine with the breaking API change in 2.8.0 (although it should be done in a PIP, but this is a separate story).

Regarding the naming of the new interfaces, like ClusterDataInterface

can we keep here the original Name for the user ?
and move to ClusterDataImpl the implementation ?
this way the changes in user code will be minimal

the name XXXInteface looks ungly in Java, the same would be for IClusterData

if we cannot change the implementation name, then I suggest a more Java friendly name like
ClusterDataBean for the interface and ClusterData for the implementation

but my first preference is:

ClusterData for the interface
ClusterDataImpl for the implementation

[freeznet]

@eolivelli thanks for your comments. I just have a discussion with @codelipenghui earlier today, and we are on the same page. I will push some commits to change the class name, like change ClusterData to ClusterDataImpl as the implementation, and change ClusterDataInterface to ClusterData as the interface.

[freeznet]

/pulsarbot run-failure-checks

[freeznet]

@merlimat @codelipenghui @eolivelli please help to review this PR if you have time, thanks.

[codelipenghui]

LGTM

[jiazhai]

@eolivelli Would you please help review this PR again?

[eolivelli]

Sorry for late reply.
I am thinking more about this patch.

A typical client application will use something like this:
admin.clusters().createCluster("test", new ClusterDataImpl(.....));

This makes me think that even if we are now allowing to use pure interfaces in order to define the API, this API is not usable without the "Impl" classes.

So, what is the benefit of splitting the API classes from Impl ?

It looks like that it will make things more complicated for the user.

Therefore....
will it be allowed to run something like:

class MyClusterData implements ClusterData {
.....
}

admin.clusters().createCluster("test", new MyClusterData());

Probably we have to think more about this big breaking change, especially now that we are close to releasing 2.8.0.

[freeznet]

@eolivelli thanks for your comments again.

This makes me think that even if we are now allowing to use pure interfaces in order to define the API, this API is not usable without the "Impl" classes.

Interfaces are intended to define the APIs and specifications, which of cause not usable without the "impl" classes. So I don't think this is a problem here.

So, what is the benefit of splitting the API classes from Impl ?

The main purpose of this PR is to solve #9640, and I am not splitting all the API classes into interface and impl class here, only those classes that will bring external dependencies into pulsar-client-admin-api. @lhotari has made a very clear description in #9640, and the discussion between @lhotari and @jerrypeng in #9638 is also very helpful to get to the context.

will it be allowed to run something like:

class MyClusterData implements ClusterData {
.....
}

admin.clusters().createCluster("test", new MyClusterData());

Actually, I do not get this point very clear, it would be great if you could give me more background about how you came up with this idea?

Probably we have to think more about this big breaking change, especially now that we are close to releasing 2.8.0.

You are right, so welcome to point it out if you have any other specified concerns about this breaking change. You can also check here for some more details about the discussion on Pulsar Community Meeting.

[eolivelli]

1. Actually, I do not get this point very clear, it would be great if you could give me more background about how you came up with this idea?

You can try to write a simple client program that uses the new Admin API without using the "Impl" classes.
If we model the POJOs/Beans with interfaces then the user may try to use a custom implementation of the interface.

Having interfaces makes sense for the users who "read" data, actually the implementation will be provided by the runtime.
But when you come to use APIs (like createCluster) that need an implementation of the interface then you are stuck in using something that is not part of the "public API".
The Public API must be the only thing that you need in order to use the API.

createCluster wants an implementation of ClusterData but you cannot provide it if you do not use the *Impl classes.
I am assuming that the *Impl classes are "implementation details", and must not used explicitly but the client.
otherwise we are not really splitting the API in some "API part" (public) and "Implementation part" (hidden, not usable directly)

if we want to go this way we must provide builders or factories for all of the new interfaces, like we do with the Schema API (for instance Schema.AUTO_CONSUME());

Apart from this problem, this is a big breaking change, that we should do with care, and probably it is not safe to do it for 2.8.0 and we have to postpone it to 2.9.0 (or even 3.0.0??)

I believe the this is one of the cases where a PIP is a good way of working (the community meeting is informal and not everyone is able to attend)

[sijie]

My take here:

We are in a situation that we can’t roll back any of this change *easily. This change involves many PRs from many modules including client, client-admin, and functions. The earliest PR related to this change is probably a couple of months ago. So we need this change to land for 2.8.0

It is already a breaking change that we need to accept for pulsar-client-admin. It is hard to get to a very clean and ideal state in one release. I see what the PR proposed is the *best middle-ground situation that we can get between the 2.8.0 release and subsequent releases.

To unblock the 2.8.0 release, I think we can add @evolving annotation to the pulsar client admin API to indicate there will be subsequent changes.

[eolivelli]

Can we add some builder/factory methods in order to help user code to create instances of the new interfaces without depending on the *Impl classes.

Alternatively we could change the name for instance from ClusterDataImpl to ClusterDataBean, following Java naming conventions and state Javadocs how to use them

[eolivelli]

I am approving this patch in order to unblock it

I would like to see some factory/builder methods in the API to let the user instantiate the internal beans without depending directly on the Impl classes.

[merlimat]

I cannot fix the conflicts here because I cannot push the branch. I'll push a new PR with this changes.