Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation" #10827

Merged
merged 1 commit into from
Jun 6, 2021
Merged

ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation" #10827

merged 1 commit into from
Jun 6, 2021

Conversation

eolivelli
Copy link
Contributor

Motivation

With recent changes in Pulsar IO we are bundling the Pulsar Client Implementation in the classpath of the Functions/Pulsar IO connectors.
Sometimes it happens that calls to DefaultImplementation fail, like this call to SchemaBuilder.record():


Caused by: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.ClassNotFoundException: org.apache.pulsar.client.impl.schema.RecordSchemaBuilderImpl
	at org.apache.pulsar.client.internal.ReflectionUtils.catchExceptions(ReflectionUtils.java:46) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.DefaultImplementation.newRecordSchemaBuilder(DefaultImplementation.java:356) ~[java-instance.jar:?]
	at org.apache.pulsar.client.api.schema.SchemaBuilder.record(SchemaBuilder.java:39) ~[java-instance.jar:?]
	at com.datastax.oss.pulsar.source.converters.AbstractGenericConverter.<init>(AbstractGenericConverter.java:58) ~[?:?]
	at com.datastax.oss.pulsar.source.converters.AvroConverter.<init>(AvroConverter.java:30) ~[?:?]
	... 10 more
Caused by: java.lang.RuntimeException: java.lang.ClassNotFoundException: org.apache.pulsar.client.impl.schema.RecordSchemaBuilderImpl
	at org.apache.pulsar.client.internal.ReflectionUtils.newClassInstance(ReflectionUtils.java:63) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.getConstructor(ReflectionUtils.java:69) ~[java-instance.jar:?]

Modifications

Using Class.forName allows Java classes loaded in the Functions Runtime to fully use the Implementation classes loaded from the Pulsar API using DefaultImplementation

Verifying this change

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

@eolivelli
ReflectionUtils use Class.forName in order to properly discover class…
dd08745 
…es in Functions Runtime while using DefaultImplementation

Using Class.forName allows Java classes loaded in the Functions Runtime to fully use the Implementation classes loaded from the Pulsar API
using DefaultImplementation
@eolivelli eolivelli self-assigned this Jun 4, 2021
@eolivelli
Copy link
Contributor Author

@codelipenghui it happens that without this patch some connectors that worked on Pulsar 2.7.x do not work anymore on 2.8.0.
please take this patch into consideration for 2.8.0 release

@codelipenghui codelipenghui requested a review from sijie June 4, 2021 12:43
freeznet
freeznet reviewed Jun 5, 2021
View reviewed changes
pulsar-client-api/src/main/java/org/apache/pulsar/client/internal/ReflectionUtils.java
@@ -51,12 +51,12 @@
try {
try {
// when the API is loaded in the same classloader as the impl
return (Class<T>) DefaultImplementation.class.getClassLoader().loadClass(className);
return (Class<T>) Class.forName(className, true, DefaultImplementation.class.getClassLoader());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set initialize to true here? Will it change the behaviour here since .loadClass(className) will not do the initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freeznet in my experience I saw that it is better to fully initialise the classes in environments where there are dynamic classloaders, because you may see weird errors when you shutdown the classloader and you still try to use the class: if the class is not fully loaded you can see NoClassDefFound errors or other link related errors.

I can try to use 'false' and test again my code if you feel strong

freeznet
freeznet reviewed Jun 5, 2021
View reviewed changes
Copy link
Contributor

@freeznet freeznet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using Class.forName as a backup solution when loadClass thrown exceptions, since you mentioned that the problem only happened sometimes.

@merlimat
Copy link
Contributor

merlimat commented Jun 5, 2021

With recent changes in Pulsar IO we are bundling the Pulsar Client Implementation in the classpath of the Functions/Pulsar IO connectors.

Is it still true in the current master? There shouldn't be anymore any client impl dependency in the function/io class path.

@eolivelli
Copy link
Contributor Author

With recent changes in Pulsar IO we are bundling the Pulsar Client Implementation in the classpath of the Functions/Pulsar IO connectors.

Is it still true in the current master? There shouldn't be anymore any client impl dependency in the function/io class path.

@merlimat
see here
https://github.com/apache/pulsar/blob/master/pulsar-functions/runtime-all/pom.xml#L51

This is the jar that is in classpath for every Function/Source/Sink.

it makes sense to have it in the classpath, but I do not know why, without this change some API functions (like the ones I cited in the description) do not work

@eolivelli
Copy link
Contributor Author

@merlimat @codelipenghui I sincerely believe that this may be a show stopper for 2.8.0 release

@codelipenghui
Copy link
Contributor

I'm not sure if there are some changes related to the classloader of the Pulsar Functions, seems the class loader of the DefaultImplementation.class changed between 2.7.x to the master branch.

@codelipenghui codelipenghui added this to the 2.8.0 milestone Jun 6, 2021
@codelipenghui codelipenghui added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Jun 6, 2021
@eolivelli
Copy link
Contributor Author

@codelipenghui probably you are right.
This fix is the smallest one I found that does not break anything.
Also it makes sense to load the classes this way.
So I believe that this change is safe to be committed now

Writing an integration test that demonstrate the failure is pretty time consuming.
In my case for instance I have a Source that is reading from a pulsar Topic with a KV schema and then returning a KVRecord.

Another error I see in a Source that uses RecordSchemaBuilder

@codelipenghui
Copy link
Contributor

@eolivelli SGTM, @merlimat @freeznet Please help check again.

@merlimat merlimat merged commit 4d263ff into apache:master Jun 6, 2021
@eolivelli eolivelli mentioned this pull request Jun 11, 2021
Merged
@eolivelli eolivelli deleted the fix/reflectionutils-loadclass branch June 11, 2021 11:44
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
@eolivelli @yangl
ReflectionUtils use Class.forName in order to properly discover class…
207dac3 
…es in Functions Runtime while using DefaultImplementation (apache#10827)

Using Class.forName allows Java classes loaded in the Functions Runtime to fully use the Implementation classes loaded from the Pulsar API
using DefaultImplementation

Co-authored-by: Enrico Olivelli <eolivelli@apache.org>
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@eolivelli @ciaocloud
ReflectionUtils use Class.forName in order to properly discover class…
2f7a0a2 
…es in Functions Runtime while using DefaultImplementation (apache#10827)

Using Class.forName allows Java classes loaded in the Functions Runtime to fully use the Implementation classes loaded from the Pulsar API
using DefaultImplementation

Co-authored-by: Enrico Olivelli <eolivelli@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freeznet freeznet freeznet left review comments

@merlimat merlimat merlimat approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@jerrypeng jerrypeng Awaiting requested review from jerrypeng

@sijie sijie Awaiting requested review from sijie

Assignees

@eolivelli eolivelli

Labels
release/blocker Indicate the PR or issue that should block the release until it gets resolved
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eolivelli @merlimat @codelipenghui @freeznet