You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/04 11:16:30 UTC
[GitHub] [pulsar] eolivelli opened a new pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
eolivelli opened a new pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855380677
@merlimat @codelipenghui I sincerely believe that this may be a show stopper for 2.8.0 release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855380204
> > 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on a change in pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
freeznet commented on a change in pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#discussion_r645941166
##########
File path: 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());
Review comment:
Why set `initialize` to `true` here? Will it change the behaviour here since `.loadClass(className)` will not do the initialization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855394108
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855403575
@eolivelli SGTM, @merlimat @freeznet Please help check again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855400990
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-854628063
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#issuecomment-855192152
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10827: ReflectionUtils use Class.forName in order to properly discover classes in Functions Runtime while using "DefaultImplementation"
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10827:
URL: https://github.com/apache/pulsar/pull/10827#discussion_r646115728
##########
File path: 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());
Review comment:
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org