[improve][fn] Use functions classloader in TopicSchema.newSchemaInstance() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) #20115

gmiklos-ltg · 2023-04-17T09:23:41Z

Motivation

While evaluating Pulsar Functions for our data transformation use-cases we ran into the following startup issue with our function that was using a custom SerDe class:
2023-04-12T11:30:35,368+0200 [public/default/user-transform-new-2-0] ERROR org.apache.pulsar.functions.instance.JavaInstanceRunnable - [public/default/user-transform-new-2:0] Uncaught exception in Java Instance java.lang.RuntimeException: User class must be in class path at org.apache.pulsar.common.util.Reflections.createInstance(Reflections.java:72) ~[org.apache.pulsar-pulsar-common-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.InstanceUtils.createInstance(InstanceUtils.java:92) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.InstanceUtils.initializeSerDe(InstanceUtils.java:51) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.source.TopicSchema.newSchemaInstance(TopicSchema.java:238) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.source.TopicSchema.newSchemaInstance(TopicSchema.java:246) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.source.TopicSchema.lambda$getSchema$0(TopicSchema.java:68) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at java.util.HashMap.computeIfAbsent(HashMap.java:1220) ~[?:?] at org.apache.pulsar.functions.source.TopicSchema.getSchema(TopicSchema.java:68) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.source.PulsarSource.buildPulsarSourceConsumerConfig(PulsarSource.java:176) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.source.SingleConsumerPulsarSource.open(SingleConsumerPulsarSource.java:69) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setupInput(JavaInstanceRunnable.java:774) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setup(JavaInstanceRunnable.java:226) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.run(JavaInstanceRunnable.java:259) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at java.lang.Thread.run(Thread.java:833) ~[?:?] Caused by: java.lang.ClassNotFoundException: com.bridge.data.UserMessageSerDe at java.net.URLClassLoader.findClass(URLClassLoader.java:445) ~[?:?] at java.lang.ClassLoader.loadClass(ClassLoader.java:587) ~[?:?] at java.lang.ClassLoader.loadClass(ClassLoader.java:520) ~[?:?] at java.lang.Class.forName0(Native Method) ~[?:?] at java.lang.Class.forName(Class.java:467) ~[?:?] at org.apache.pulsar.common.util.Reflections.createInstance(Reflections.java:70) ~[org.apache.pulsar-pulsar-common-2.11.0.jar:2.11.0] ... 13 more

We made it absolutely sure that the class is included in the fat jar that we have compiled.

config.yaml looks like this:

tenant: public
namespace: default
name: user-transform
className: com.bridge.data.TransformUserMessage
inputs:
  - "persistent://public/default/debezium.public.users"
customSerdeInputs:
  "persistent://public/default/debezium.public.users": com.bridge.data.UserMessageSerDe
output: "persistent://public/default/debezium.public.users-public"
outputSerdeClassName: com.bridge.data.PublicUserSerDe
jar: "transform-function-v1.jar"
parallelism: 1
retainOrdering: true

The mentioned UserMessageSerDe class is just a very simple wrapper around Jackson to deserialize UserMessage data classes.

Modifications

I have modified TopicSchema in such a way that it accepts a functions classloader parameter which will be used by newSchemaInstance() to resolve this class. This way our function can now start up without any issues.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as TopicSchemaTest.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

tisonkun · 2023-04-19T16:04:39Z

pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/source/TopicSchema.java

        this.client = client;
+        this.functionsClassloader = AccessController.doPrivileged(


Why do we need AccessController.doPrivileged here?

Interestingly IntelliJ IDEA gives me a pretty strong warning that doPrivileged should be used while creating this URLClassLoader. Shall we omit this call and just create the URLClassLoader as it is? I saw other parts of code that made use of doPrivileged (perhaps it also makes static analysis to fail).

This method (as well as AccessController) is deprecated to be removed since JDK 17. We should find other way or just omit the warning.

See https://openjdk.org/jeps/411

tisonkun · 2023-04-19T16:17:47Z

pulsar-functions/instance/src/test/java/org/apache/pulsar/functions/source/TopicSchemaTest.java

Is it possible for us to add a test case fails on the current master branch? Maybe follow the case introduced in #5357 so we know what is fixed and prevent regression.

I'll see what I can do.

I have expanded upon PulsarFunctionsTest to also include an input SerDe class. It should fail on the current master because as it was similarly with #5357 it uses the wrong classloader to load the input SerDe class.

…lassNotFoundException when using custom SerDe classes.

Fix license header issue.

tisonkun · 2023-05-05T09:02:32Z

/pulsarbot run-failure-tests

tisonkun · 2023-05-08T08:37:07Z

/pulsarbot run-failure-checks

nlu90 · 2023-05-15T17:24:59Z

/pulsarbot run-failure-checks

nlu90 · 2023-05-15T23:35:12Z

/pulsarbot run-failure-checks

codecov-commenter · 2023-05-16T16:47:39Z

Codecov Report

Merging #20115 (3150370) into master (9841364) will decrease coverage by 0.71%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master   #20115      +/-   ##
============================================
- Coverage     72.87%   72.16%   -0.71%     
- Complexity    31943    31957      +14     
============================================
  Files          1868     1869       +1     
  Lines        138594   142047    +3453     
  Branches      15246    15996     +750     
============================================
+ Hits         101003   102512    +1509     
- Misses        29547    31367    +1820     
- Partials       8044     8168     +124

Flag	Coverage Δ
inttests	`24.61% <0.00%> (+0.34%)`	⬆️
systests	`25.11% <100.00%> (+0.18%)`	⬆️
unittests	`72.22% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../apache/pulsar/functions/instance/ContextImpl.java	`61.19% <100.00%> (ø)`
...a/org/apache/pulsar/functions/sink/PulsarSink.java	`73.85% <100.00%> (ø)`
...g/apache/pulsar/functions/source/PulsarSource.java	`77.01% <100.00%> (ø)`
...r/functions/source/SingleConsumerPulsarSource.java	`93.33% <100.00%> (ø)`
...rg/apache/pulsar/functions/source/TopicSchema.java	`68.35% <100.00%> (+0.40%)`	⬆️

... and 94 files with indirect coverage changes

nlu90 · 2023-05-18T04:01:43Z

@tisonkun do you have any other concerns or comments about the PR?

If no, we'll approve and merge it.

tisonkun

LGTM

@nlu90 your approval should be isolated from mine, lol.

…nce() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) (#20115)

…nce() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) (#20115) (cherry picked from commit 43a9898)

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 17, 2023

gmiklos-ltg mentioned this pull request Apr 17, 2023

[fix][fn] Use functions classloader in TopicSchema #20090

Closed

15 tasks

tisonkun requested review from nlu90 and tisonkun April 19, 2023 07:00

tisonkun reviewed Apr 19, 2023

View reviewed changes

gmiklos-ltg added 3 commits April 28, 2023 13:43

Use functions classloader in TopicSchema.newSchemaInstance() to fix C…

8b6305b

…lassNotFoundException when using custom SerDe classes.

Fix code style.

1ed3373

Add input serde parameter in PulsarFunctionsJavaTest

c417377

Fix license header issue.

gmiklos-ltg force-pushed the serde-source-classloading-master branch from 535291c to c417377 Compare April 28, 2023 13:32

tisonkun self-requested a review May 5, 2023 08:42

tisonkun added the ready-to-test label May 5, 2023

Merge branch 'master' into serde-source-classloading-master

3150370

nlu90 assigned gmiklos-ltg May 15, 2023

nlu90 closed this May 15, 2023

nlu90 reopened this May 15, 2023

tisonkun approved these changes May 18, 2023

View reviewed changes

Technoboy- added this to the 3.1.0 milestone May 18, 2023

Technoboy- added area/function release/2.11.2 release/3.0.1 labels May 18, 2023

Technoboy- merged commit 43a9898 into apache:master May 18, 2023

Technoboy- added the cherry-picked/branch-2.11 label May 24, 2023

Technoboy- pushed a commit that referenced this pull request May 24, 2023

[improve][fn] Use functions classloader in TopicSchema.newSchemaInsta…

9d813d0

…nce() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) (#20115)

poorbarcode added the cherry-picked/branch-3.0 label May 30, 2023

freeznet mentioned this pull request Apr 15, 2024

[fix][fn]make sure the classloader for ContextImpl is functionClassLoader in different runtimes #22501

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][fn] Use functions classloader in TopicSchema.newSchemaInstance() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) #20115

[improve][fn] Use functions classloader in TopicSchema.newSchemaInstance() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) #20115

gmiklos-ltg commented Apr 17, 2023

tisonkun Apr 19, 2023

gmiklos-ltg Apr 28, 2023

tisonkun May 8, 2023

tisonkun May 8, 2023

tisonkun Apr 19, 2023 •

edited

Loading

gmiklos-ltg Apr 28, 2023

gmiklos-ltg May 2, 2023

tisonkun commented May 5, 2023 •

edited

Loading

tisonkun commented May 8, 2023

nlu90 commented May 15, 2023

nlu90 commented May 15, 2023

codecov-commenter commented May 16, 2023

nlu90 commented May 18, 2023

tisonkun left a comment

		this.client = client;
		this.functionsClassloader = AccessController.doPrivileged(

[improve][fn] Use functions classloader in TopicSchema.newSchemaInstance() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) #20115

[improve][fn] Use functions classloader in TopicSchema.newSchemaInstance() to fix ClassNotFoundException when using custom SerDe classes. (targeted for master) #20115

Conversation

gmiklos-ltg commented Apr 17, 2023

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

tisonkun Apr 19, 2023

Choose a reason for hiding this comment

gmiklos-ltg Apr 28, 2023

Choose a reason for hiding this comment

tisonkun May 8, 2023

Choose a reason for hiding this comment

tisonkun May 8, 2023

Choose a reason for hiding this comment

tisonkun Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

gmiklos-ltg Apr 28, 2023

Choose a reason for hiding this comment

gmiklos-ltg May 2, 2023

Choose a reason for hiding this comment

tisonkun commented May 5, 2023 • edited Loading

tisonkun commented May 8, 2023

nlu90 commented May 15, 2023

nlu90 commented May 15, 2023

codecov-commenter commented May 16, 2023

Codecov Report

nlu90 commented May 18, 2023

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun Apr 19, 2023 •

edited

Loading

tisonkun commented May 5, 2023 •

edited

Loading