Java Client: remove usage of reflection while using Pulsar Implementation classes #11636

eolivelli · 2021-08-11T07:20:32Z

load the DefaultImplementation only once with reflection
use standard Java calls to access the Pulsar Implementation classes
reduce the overall overhead of using Reflection
clarify the relationship between the API and the Impl classes
add explicit linking between the impl and api classes, this way it is easier to read the code and perform refactorings
allow to handle correctly the Exception thrown by implementation classes

Motivation

Currently the link between the Pulsar Client API and the Implementation is done at runtime, using Java reflection, in the DefaultImplementation class.

This comes with a few problems:

it is not clear the Classloader who is loading the classes of the Implementation, DefaultImplementation relied on "RelectionUtils", with surprising effects we saw during the past months, expecially in Pulsar Functions/Pulsar II
the old DefaultImplementation class used to catch all the checked exception and hide them
the old reflection approach used "strings" to refer to implementation classes, making it harder to perform refactorings, understand the code flow (by reading), using the help of the IDE
using reflection is generally "slower", with this fix we are going to save CPU cycles, and also the JVM will be able to understand better what's happening and (probably) perform more runtime optimizations (at least we are going to save a few method calls and stack...)

Modifications

Introduce a new Java interface PulsarClientImplementationBinding that defines the connections from the "public API" and the "implementation", this interface contains all the methods of the old "DefaultImplementation"
Move the Implementation to the pulsar-client module, in the concrete class PulsarClientImplementationBindingImpl
PulsarClientImplementationBindingImpl can access directly, without reflection all the implementation classes, this is faster and also it allows us to have direct linking (for human reading, for IDE help....)
Load the PulsarClientImplementationBindingImpl class only once
Rewrite all the references to the old DefaultImplementation.staticMethod(xxx) to DefaultImplementation.getDefaultImplementation().method(xxx)
Fix some places in which hidden IOException may have bubbled up breaking things without notice

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Documentation

No need for docs

- load the DefaultImplementation only once with reflection - use standard Java calls to access the Pulsar Implementation classes - reduce the overall overhead of using Reflection - clarify the relationsship between the classes - add explicit linking between the impl and api classes, this way it is easier to read the code and perform refactorings - allow to handle correctly the Exception thrown by implementation classes

rdhabalia

LGTM

merlimat

I really like this approach. I just have a question on how we can guarantee that we're picking the right classloader.

merlimat · 2021-08-11T20:36:13Z

...lient/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java

+    }
+
+    public ClientBuilder newClientBuilder() {
+        return new org.apache.pulsar.client.impl.ClientBuilderImpl();


Wouldn't this be using the class loader from the current context? How can we be sure it's going to be the same as PulsarClientImplementationBindingImpl?

See the JVM specs here
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-5.html
chapter "5.3. Creation and Loading"
look for "If D was defined by a user-defined class loader, then that same user-defined class loader initiates loading of C"

basically if a class "D" (where "D" is not a system class) triggers the loading of a class "C", then the same Classloader that loaded "D" will initiate the loading of "C"

So in this case the same classloader which loaded PulsarClientImplementationBindingImpl will initiate the loading of ClientBuilderImpl if the class has not been loaded yet

Got it. thanks!

merlimat · 2021-08-11T20:36:44Z

...lient/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java

+    }
+
+    public MessageId newMessageId(long ledgerId, long entryId, int partitionIndex) {
+        return new org.apache.pulsar.client.impl.MessageIdImpl(ledgerId, entryId, partitionIndex);


Couldn't we import the classes here instead of using the full name?

Fixed.

I left the FQCN only because I converted the class semi automatically, before of this change they where simple strings.

Anonymitaet · 2021-08-12T00:18:18Z

@eolivelli thanks for providing doc-related info. If your PR does not need to update doc, could you please help label the PR with no-need-doc in the future? Thanks

eolivelli · 2021-08-12T08:41:11Z

@eolivelli thanks for providing doc-related info. If your PR does not need to update doc, could you please help label the PR with no-need-doc in the future? Thanks
sure. thanks for the reminder

eolivelli · 2021-08-12T09:10:08Z

@merlimat I have removed the usage of Fully qualified class names.
I have answered to your question about classloading (I had to verify this in the specs).

After merging this patch I will add new integration tests in Pulsar Functions in order to verify that we are loading correctly all (of most of them) the classes referred by PulsarClientImplementationBindingImpl (Schema.XXX, RecordSchemaBuilder, BatcherBuilder, KeyValueSchema....)

…tion classes (apache#11636) * Java Client: remove usage of reflection for loading Pulsar Impl classes - load the DefaultImplementation only once with reflection - use standard Java calls to access the Pulsar Implementation classes - reduce the overall overhead of using Reflection - clarify the relationsship between the classes - add explicit linking between the impl and api classes, this way it is easier to read the code and perform refactorings - allow to handle correctly the Exception thrown by implementation classes * Fix checkstyle and build * fix unit test * Add some javadoc and remove use of fully qualified class names Co-authored-by: Enrico Olivelli <eolivelli@apache.org>

…tion classes (apache#11636) * Java Client: remove usage of reflection for loading Pulsar Impl classes - load the DefaultImplementation only once with reflection - use standard Java calls to access the Pulsar Implementation classes - reduce the overall overhead of using Reflection - clarify the relationsship between the classes - add explicit linking between the impl and api classes, this way it is easier to read the code and perform refactorings - allow to handle correctly the Exception thrown by implementation classes * Fix checkstyle and build * fix unit test * Add some javadoc and remove use of fully qualified class names Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 683c2d6)

eolivelli added area/client area/function area/connector java labels Aug 11, 2021

eolivelli added this to the 2.9.0 milestone Aug 11, 2021

eolivelli requested review from codelipenghui, jerrypeng, merlimat, rdhabalia and sijie August 11, 2021 07:20

eolivelli self-assigned this Aug 11, 2021

Fix checkstyle and build

ba6b9f9

eolivelli mentioned this pull request Aug 11, 2021

Explicitly set the Pulsar function classloader #10922

Closed

eolivelli requested a review from lhotari August 11, 2021 07:33

fix unit test

21cbb23

eolivelli marked this pull request as ready for review August 11, 2021 08:50

eolivelli requested a review from ivankelly August 11, 2021 14:39

eolivelli changed the title ~~Java Client: remove usage of reflection for loading Pulsar Implementation classes~~ Java Client: remove usage of reflection while using Pulsar Implementation classes Aug 11, 2021

rdhabalia approved these changes Aug 11, 2021

View reviewed changes

merlimat reviewed Aug 11, 2021

View reviewed changes

Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Aug 12, 2021

Add some javadoc and remove use of fully qualified class names

9448da3

merlimat approved these changes Aug 12, 2021

View reviewed changes

merlimat merged commit 683c2d6 into apache:master Aug 12, 2021

eolivelli deleted the impl/api-impl-binding branch March 24, 2022 12:28

eolivelli mentioned this pull request Mar 31, 2022

Cannot use org.apache.avro.generic.GenericRecord inside a Pulsar Sink (to manage GenericRecord.getNativeObject) #11274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java Client: remove usage of reflection while using Pulsar Implementation classes #11636

Java Client: remove usage of reflection while using Pulsar Implementation classes #11636

Uh oh!

eolivelli commented Aug 11, 2021 •

edited

Loading

Uh oh!

rdhabalia left a comment

Uh oh!

merlimat left a comment

Uh oh!

merlimat Aug 11, 2021

Uh oh!

eolivelli Aug 12, 2021

Uh oh!

merlimat Aug 12, 2021

Uh oh!

merlimat Aug 11, 2021

Uh oh!

eolivelli Aug 12, 2021

Uh oh!

Anonymitaet commented Aug 12, 2021

Uh oh!

eolivelli commented Aug 12, 2021

Uh oh!

eolivelli commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Java Client: remove usage of reflection while using Pulsar Implementation classes #11636

Java Client: remove usage of reflection while using Pulsar Implementation classes #11636

Uh oh!

Conversation

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

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

rdhabalia left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

eolivelli Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

merlimat Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

merlimat Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

eolivelli Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

Anonymitaet commented Aug 12, 2021

Uh oh!

eolivelli commented Aug 12, 2021

Uh oh!

eolivelli commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eolivelli commented Aug 11, 2021 •

edited

Loading