[trans to #10563][fix #9640] remove `pulsar-client-admin-api` dependency : `pulsar-client-original` #10513

freeznet · 2021-05-08T08:30:34Z

Fixes #9640
Part 1

Motivation

Module pulsar-client-admin-api has been introduced in #9246, but it pulling in many dependencies. That is undesirable for an "API" module. As @sijie @merlimat and @jerrypeng suggests, this pr resolves the issue and make pulsar-client-admin-api has minimal transient dependences.

This PR removes pulsar-client-original from the dependency, and resolves the issues. Later there will be sepreate PRs to remove other dependencies like pulsar-common and pulsar-package-core.

I will close #9842 and keep update here.

Modifications

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

Verifying this change

Make sure that the change passes the CI checks.

freeznet · 2021-05-08T13:59:25Z

/pulsarbot run-failure-checks

freeznet · 2021-05-10T12:34:33Z

/pulsarbot run-failure-checks

freeznet · 2021-05-11T04:13:21Z

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

merlimat · 2021-05-11T04:31:18Z

pulsar-io/kafka/pom.xml


+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>pulsar-client-original</artifactId>


Why do we need pulsar client explicitly for Kafka connector?

@merlimat I believe this is related to the conversion between pulsar's schema and kafka's schema. So kafka connector need pulsar-client-original which all schema impls are in the package.

yes, @freeznet is right. we need Schema Impl classes

merlimat · 2021-05-11T04:32:44Z

pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java

                    @Override
-                    public void completed(OffloadProcessStatus offloadProcessStatus) {
+                    public void completed(String jsonString) {
+                        Gson gson = new GsonBuilder().registerTypeAdapter(MessageId.class,


We should use Jackson for all the JSON ser/de, unless there's a particular reason not to do so. Please use ObjectMapperFactory.getThreadLocal()....

Thanks for your suggestion. The reason why I use Gson here is that I noticed TopicsImpl.java is using Gson to de REST response to JsonObject already, like getInternalInfoAsync, so I havent try Jackson here to avoid additional imports.

And in this case, we want de MessageId.class to MessageIdImpl.class, Gson have a simpler way to map interface (MessageId.class) with implementation class (MessageIdImpl.class), for Jackson we cannot use @JsonDeserialize annotation because we dont want to have Jackson dependency in pulsar-client-admin-api, but I have googled with a solution with SimpleAbstractTypeResolver, like

ObjectMapper mapper = ObjectMapperFactory.create(); SimpleModule module = new SimpleModule("OffloadProcessStatusConvertModule", Version.unknownVersion()); SimpleAbstractTypeResolver resolver = new SimpleAbstractTypeResolver(); resolver.addMapping(MessageId.class, MessageIdImpl.class); module.setAbstractTypes(resolver); mapper.registerModule(module);

So if we want to use Jackson here, we may try this way.

Also I do confuse with Gson and Jackson here in Pulsar, do we want to replace Gson with Jackson, or we will keep the mixed usage but prefer Jackson by default? And what are the concerns?

freeznet@fa05e38
here is a sample commit that changes Gson to Jackson.

We want to make sure to use Jackson, through the ObjectMapperFactory.getThreadLocal() since it comes configured with some settings.

If @JsonDeserialize annotations are the problem, there will be bigger problems porting the POJOs defined in pulsar-common.

The better option here is to untie the pulsar-client-admin-api from the POJOs. Instead, pulsar-client-admin-api should return Java interfaces instead of POJOs that are used for serialization.

@merlimat Indeed, there will be some problems with pulsar-common. But if we return Java interfaces instead of POJOs, we still need to have a mapping list to have Java Interface -> POJO so that Jackson can do the deserialization.

pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/PulsarAdmin.java

eolivelli

LGTM

pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/PulsarAdmin.java

eolivelli · 2021-05-12T07:45:38Z

pulsar-io/kafka/pom.xml


+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>pulsar-client-original</artifactId>


yes, @freeznet is right. we need Schema Impl classes

freeznet · 2021-05-12T08:43:22Z

...-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/JacksonConfigurator.java

+        // because we do not want to have jackson dependency in pulsar-client-admin-api
+        // In this case we use SimpleAbstractTypeResolver to map interfaces to impls
+        SimpleAbstractTypeResolver resolver = new SimpleAbstractTypeResolver();
+        resolver.addMapping(OffloadProcessStatus.class, OffloadProcessStatusImpl.class);


@merlimat I have added a SimpleAbstractTypeResolver here, and will manually map interfaces with impl classes.

freeznet · 2021-05-12T09:33:46Z

This PR introduced a new dependency gson to pulsar-client-admin-api, I will remove it in a separate PR.

eolivelli · 2021-05-12T11:24:51Z

pulsar-client-admin-api/pom.xml

-            <artifactId>pulsar-client-original</artifactId>
-            <version>${project.parent.version}</version>
+            <groupId>com.google.code.gson</groupId>
+            <artifactId>gson</artifactId>


if gson is not already a dependency imported by pulsar-client-original in 2.7.x I would like to not add it.
Users of pulsar-client-original already have Jackson and usually who uses pulsar-client-admin-api uses pulsar-client, so I would lean toward using only Jackson.

what's your plan for removing gson in a follow up patch ? this next patch will become a blocker for the 2.8 release

Adding gson as the dependency is a temporary action, with removal of pulsar-client-original and pulsar-package-core, pulsar-client-admin-api will lose dependency of gson, but we still have a lot of classes import gson, such as

pulsar/pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java

Line 21 in 581fd5b

import com.google.gson.JsonObject;

.

I do understand that the 2.8 release is in a hurry, but there are indeed many problems that need to be fixed one by one in order to achieve the ultimate goal. So the removal of gson will be in a separate PR soon.

My problem was about "adding" gson, if it was not already there, so to me this is not a problem.

thanks for your clarification

freeznet · 2021-05-12T11:40:34Z

/pulsarbot run-failure-checks

eolivelli

LGTM

…ule for interface resolver # Conflicts: # pulsar-client-admin-api/pom.xml

…y-pulsar-client-original

freeznet · 2021-05-13T03:40:33Z

due to my mistaken ops on the branch, this PR has been closed, so please move to #10563, sorry for the inconvenience.

freeznet mentioned this pull request May 8, 2021

[WIP] [fix #9640] fix pulsar-client-admin-api dependency #9842

Closed

5 tasks

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch from 2dd33ad to 2ef02f5 Compare May 8, 2021 12:46

freeznet changed the title ~~[fix #9640] remove client admin api dependency pulsar client original / part 1~~ [fix #9640] remove pulsar-client-admin-api dependency : pulsar-client-original May 8, 2021

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch from f4601e6 to 13b8f5c Compare May 10, 2021 09:38

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch 2 times, most recently from bf799e3 to 8f45944 Compare May 11, 2021 02:52

merlimat reviewed May 11, 2021

View reviewed changes

freeznet mentioned this pull request May 11, 2021

[fix #9640] remove pulsar-client-admin-api dependency : pulsar-package-core #10538

Merged

1 task

merlimat added this to the 2.8.0 milestone May 11, 2021

merlimat added the area/build label May 11, 2021

merlimat assigned freeznet May 11, 2021

eolivelli requested changes May 11, 2021

View reviewed changes

pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/PulsarAdmin.java Show resolved Hide resolved

merlimat added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label May 11, 2021

eolivelli approved these changes May 12, 2021

View reviewed changes

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch 2 times, most recently from a1f878f to e5fd1b3 Compare May 12, 2021 08:39

freeznet commented May 12, 2021

View reviewed changes

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch from e5fd1b3 to 747d7fd Compare May 12, 2021 09:10

eolivelli requested changes May 12, 2021

View reviewed changes

eolivelli approved these changes May 12, 2021

View reviewed changes

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch from 747d7fd to 3629c7a Compare May 13, 2021 01:43

freeznet added 4 commits May 13, 2021 11:23

remove org.apache.pulsar.client.impl.conf.ClientConfigurationData

0f91214

use MessageId instead of MessageIdImpl

01efe2e

get getClientConfigData back

0acd021

fix io-kafka dependency

0d95b93

freeznet added 6 commits May 13, 2021 11:23

fix java-test-functions dependency

5080d06

add pulsar-client-original to pulsar-functions-runtime-all

c4cae81

fix offload tests

ac0de19

fix import

ef5e2de

change Gson to Jackson for OffloadProcessStatus

cabd2d0

move OffloadProcessStatus as interface, add InterfaceDefaultMapperMod…

4a7dc75

…ule for interface resolver # Conflicts: # pulsar-client-admin-api/pom.xml

freeznet force-pushed the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch from 3629c7a to 4a7dc75 Compare May 13, 2021 03:24

Merge branch 'master' into freeznet/remove-client-admin-api-dependenc…

0baad59

…y-pulsar-client-original

freeznet closed this May 13, 2021

freeznet deleted the freeznet/remove-client-admin-api-dependency-pulsar-client-original branch May 13, 2021 03:35

freeznet mentioned this pull request May 13, 2021

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

Merged

1 task

freeznet changed the title ~~[fix #9640] remove pulsar-client-admin-api dependency : pulsar-client-original~~ [trans to #10563][fix #9640] remove pulsar-client-admin-api dependency : pulsar-client-original May 13, 2021

freeznet mentioned this pull request May 25, 2021

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

Closed

1 task

[trans to #10563][fix #9640] remove pulsar-client-admin-api dependency : pulsar-client-original #10513

[trans to #10563][fix #9640] remove pulsar-client-admin-api dependency : pulsar-client-original #10513

Uh oh!

Conversation

freeznet commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Uh oh!

freeznet commented May 8, 2021

Uh oh!

freeznet commented May 10, 2021

Uh oh!

freeznet commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freeznet commented May 12, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

freeznet commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[trans to #10563][fix #9640] remove `pulsar-client-admin-api` dependency : `pulsar-client-original` #10513

[trans to #10563][fix #9640] remove `pulsar-client-admin-api` dependency : `pulsar-client-original` #10513

freeznet commented May 8, 2021 •

edited

Loading

freeznet May 11, 2021 •

edited

Loading

freeznet May 11, 2021 •

edited

Loading

freeznet May 12, 2021 •

edited

Loading

freeznet May 12, 2021 •

edited

Loading

freeznet commented May 12, 2021 •

edited

Loading