Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Jun 14, 2021

Motivation

  • tests/docker-images/java-test-functions should depend on pulsar-io-core (and pulsar-functions-api)
    • there should be no dependency on pulsar-client library directly (pulsar-client-api dependency comes via pulsar-functions-api)

Modifications

  • revisit tests/docker-images/java-test-functions/pom.xml

@lhotari lhotari added the type/bug The PR fixed a bug or issue reported a bug label Jun 14, 2021
@lhotari lhotari added this to the 2.8.0 milestone Jun 14, 2021
@lhotari lhotari requested review from eolivelli, merlimat and sijie June 14, 2021 09:00
@lhotari lhotari self-assigned this Jun 14, 2021
@eolivelli
Copy link
Contributor

This is what we would like to happen.
But IIRC integration tests were not passing.
cc @codelipenghui

I also have problems in source Pulsar IO connector if I do not bundle the full pulsar-client dependency.
I will open a separate issue

eolivelli
eolivelli previously approved these changes Jun 14, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

if it works and CI passes then LGTM

@lhotari
Copy link
Member Author

lhotari commented Jun 14, 2021

@codelipenghui @jerrypeng @eolivelli The integration tests fail

see https://github.com/apache/pulsar/pull/10918/checks?check_run_id=2818223864#step:14:4912

09:37:04.197 [public/default/test-state-source-xrveoiym-0] ERROR org.apache.pulsar.functions.instance.JavaInstanceRunnable - Source open produced uncaught exception: 
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:45) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.DefaultImplementation.newRecordSchemaBuilder(DefaultImplementation.java:355) ~[java-instance.jar:?]
	at org.apache.pulsar.client.api.schema.SchemaBuilder.record(SchemaBuilder.java:39) ~[java-instance.jar:?]
	at org.apache.pulsar.tests.integration.io.GenericRecordSource.open(GenericRecordSource.java:51) ~[java-test-functions.jar-unpacked/:?]
	at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setupInput(JavaInstanceRunnable.java:734) [org.apache.pulsar-pulsar-functions-instance-2.9.0-SNAPSHOT.jar:2.9.0-SNAPSHOT]
	at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setup(JavaInstanceRunnable.java:219) [org.apache.pulsar-pulsar-functions-instance-2.9.0-SNAPSHOT.jar:2.9.0-SNAPSHOT]
	at org.apache.pulsar.functions.instance.JavaInstanceRunnable.run(JavaInstanceRunnable.java:243) [org.apache.pulsar-pulsar-functions-instance-2.9.0-SNAPSHOT.jar:2.9.0-SNAPSHOT]
	at java.lang.Thread.run(Thread.java:829) [?:?]
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:62) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.getConstructor(ReflectionUtils.java:68) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.DefaultImplementation.lambda$newRecordSchemaBuilder$42(DefaultImplementation.java:356) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.catchExceptions(ReflectionUtils.java:34) ~[java-instance.jar:?]
	... 7 more
Caused by: java.lang.ClassNotFoundException: org.apache.pulsar.client.impl.schema.RecordSchemaBuilderImpl
	at java.net.URLClassLoader.findClass(URLClassLoader.java:471) ~[?:?]
	at java.lang.ClassLoader.loadClass(ClassLoader.java:589) ~[?:?]
	at java.lang.ClassLoader.loadClass(ClassLoader.java:522) ~[?:?]
	at java.lang.Class.forName0(Native Method) ~[?:?]
	at java.lang.Class.forName(Class.java:398) ~[?:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.newClassInstance(ReflectionUtils.java:59) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.getConstructor(ReflectionUtils.java:68) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.DefaultImplementation.lambda$newRecordSchemaBuilder$42(DefaultImplementation.java:356) ~[java-instance.jar:?]
	at org.apache.pulsar.client.internal.ReflectionUtils.catchExceptions(ReflectionUtils.java:34) ~[java-instance.jar:?]
	... 7 more

It seems that the classloading issues haven't been resolved. Isn't this a blocker for 2.8 release?

@eolivelli
Copy link
Contributor

eolivelli commented Jun 14, 2021

@lhotari for reference this is my issue
#10919

the error is the same

the fix is to add pulsar-client-original inside the NAR file in my case, that is what you are trying to undo here with this PR

@eolivelli eolivelli dismissed their stale review June 14, 2021 10:16

because we have to discuss more about this topic in the community

@lhotari
Copy link
Member Author

lhotari commented Jun 14, 2021

the fix is to add pulsar-client-original inside the NAR file in my case, that is what you are trying to undo here with this PR

the invalid shading configuration for java-test-functions was added here: https://github.com/apache/pulsar/blame/0469dfe2c7804bd9ca9ea34e95d83b2196216cf9/tests/docker-images/java-test-functions/pom.xml#L78-L85 , as part of #9246 changes. This PR fixes the invalid configuration.

@codelipenghui codelipenghui modified the milestones: 2.8.0, 2.9.0 Jun 16, 2021
@lhotari
Copy link
Member Author

lhotari commented Jun 18, 2021

@jerrypeng Please take a look at this PR (which causes integration tests to fail) and the related issue #10919 . Is this something that should be addressed? These problems are related to Pulsar Functions classloading.

@lhotari
Copy link
Member Author

lhotari commented Jun 21, 2021

UPDATE: I learned from @eolivelli that there's PR #10922 in progress which will address the Pulsar Functions classloading issue.

lhotari added a commit to lhotari/pulsar-contributor-toolbox that referenced this pull request Jul 21, 2021
@lhotari lhotari force-pushed the lh-fix-java-test-functions-dependencies branch 2 times, most recently from 2692557 to 1efbad8 Compare August 2, 2021 14:52
@eolivelli
Copy link
Contributor

@lhotari you want to try to rebase this patch to latest master ?
we committed the changes to DefaultImplementation about loading Pulsar Client Impl (mostly) without reflection

@lhotari lhotari force-pushed the lh-fix-java-test-functions-dependencies branch from 1efbad8 to dea4bcd Compare August 16, 2021 06:57
@lhotari
Copy link
Member Author

lhotari commented Aug 16, 2021

@eolivelli I rebased this PR. Let's see what the test results are.

- java-test-functions should depend on pulsar-io-core (and pulsar-functions-api)
@lhotari lhotari force-pushed the lh-fix-java-test-functions-dependencies branch from dea4bcd to 6a82b2e Compare September 9, 2021 06:00
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

can you please follow my latest comments ?
we had to add avro and jackson inthe classpath in order to see GenericRecord.getNativeObject work properly
so adding that deps is not needed anymore

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli
Copy link
Contributor

@merlimat @jerrypeng @codelipenghui
if no one objects I would like to merge this patch now.
this patch simplifies the dependencies and also makes it clear about how to use correctly Pulsar dependencies in Pulsar IO/Functions

@eolivelli eolivelli merged commit 356aa76 into apache:master Sep 13, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
- java-test-functions should depend on pulsar-io-core (and pulsar-functions-api)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants