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.

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

Kafka cannot be loaded through our ExporterJarClassLoader #4196

Closed
npepinpe opened this issue Mar 31, 2020 · 1 comment · Fixed by #7150
Closed

Kafka cannot be loaded through our ExporterJarClassLoader #4196

npepinpe opened this issue Mar 31, 2020 · 1 comment · Fixed by #7150
Assignees
Labels
kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/low Marks a bug as having little to no noticeable impact for the user

Comments

@npepinpe
Copy link
Member

Describe the bug

If loading an exporter with Kafka as a dependency through an isolated ExporterJarClassLoader, Kafka will produce ClassNotFoundError. The class, however, is definitely found by our class loader - it is simply unavailable to Kafka as it tries to load it through the Thread#getContextClassLoader(), which is (if unset) the class loader of the thread's creator - in our case, the system class loader.

To Reproduce

I used one of our user's exporter - https://github.com/openMF/ph-ee-exporter - and could reproduce the problem by loading an exporter and pointing the JAR path to the fat JAR it produces. I also verified the issue is indeed the Thread#getContextClassLoader by wrapping all of the accesses to an exporter instance's methods (e.g. #configure, #open, #close, etc.) in a try/finally block where I override the thread context class loader, and the problem went away.

Expected behavior

I expect that we support exporters providing their own dependency versions in an isolated class loader to avoid dependency collisions, and that we support most common access cases (e.g. getClass().getClassLoader(), Thread.currentThread().getContextClassLoader(), etc.).

Log/Stacktrace
If possible add the full stacktrace or Zeebe log which contains the issue.

Full Stacktrace

19:30:22.391 [Broker-0-Exporter-1] [Broker-0-zb-fs-workers-0] ERROR io.zeebe.util.actor - Uncaught exception in 'Broker-0-Exporter-1' in phase 'STARTED'. Continuing with next job.
org.apache.kafka.common.config.ConfigException: Invalid value org.apache.kafka.common.serialization.StringSerializer for configuration key.serializer: Class org.apache.kafka.common.serialization.StringSerializer could not be found.
	at org.apache.kafka.common.config.ConfigDef.parseType(ConfigDef.java:728) ~[?:?]
	at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:474) ~[?:?]
	at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:467) ~[?:?]
	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:108) ~[?:?]
	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:129) ~[?:?]
	at org.apache.kafka.clients.producer.ProducerConfig.<init>(ProducerConfig.java:409) ~[?:?]
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:326) ~[?:?]
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:270) ~[?:?]
	at hu.dpc.rt.kafkastreamer.exporter.KafkaExporterClient.<init>(KafkaExporterClient.java:57) ~[?:?]
	at hu.dpc.rt.kafkastreamer.exporter.KafkaExporter.createClient(KafkaExporter.java:77) ~[?:?]
	at hu.dpc.rt.kafkastreamer.exporter.KafkaExporter.open(KafkaExporter.java:43) ~[?:?]
	at io.zeebe.broker.exporter.stream.ExporterDirector.onSnapshotRecovered(ExporterDirector.java:225) ~[classes/:?]
	at io.zeebe.broker.exporter.stream.ExporterDirector.onActorStarted(ExporterDirector.java:140) ~[classes/:?]
	at io.zeebe.util.sched.ActorJob.invoke(ActorJob.java:73) ~[classes/:?]
	at io.zeebe.util.sched.ActorJob.execute(ActorJob.java:39) [classes/:?]
	at io.zeebe.util.sched.ActorTask.execute(ActorTask.java:115) [classes/:?]
	at io.zeebe.util.sched.ActorThread.executeCurrentTask(ActorThread.java:107) [classes/:?]
	at io.zeebe.util.sched.ActorThread.doWork(ActorThread.java:91) [classes/:?]
	at io.zeebe.util.sched.ActorThread.run(ActorThread.java:195) [classes/:?]
19:30:22.591 [GatewayTopologyManager] [Broker-0-zb-actors-0] DEBUG io.zeebe.gateway - Received metadata change from Broker 0, partitions {1=LEADER} and terms {1=12}.

@npepinpe npepinpe added Status: Ready kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog labels Mar 31, 2020
@npepinpe npepinpe self-assigned this Apr 6, 2020
@npepinpe
Copy link
Member Author

npepinpe commented Apr 6, 2020

I had some free time today to look at it, the solution is pretty straight forward, just some changes in the ExporterDirector, but testing it is...interesting 😅

There I will need some brainstorming help on how to write a good test.

@npepinpe npepinpe added severity/low Marks a bug as having little to no noticeable impact for the user Priority: Low labels Apr 28, 2020
ghost pushed a commit that referenced this issue Jun 2, 2021
7150: Fix issues with class loading for external exporters relying on the Thread's context class loader r=npepinpe a=npepinpe

## Description

This PR fixes issues when an external exporter (i.e. a self-contained JAR) wants to load classes via the current thread's context class loader. This occurred, for example, in the Kafka exporter, as the Kafka library will use the current thread's context class loader instead of just the current class loader. The fix is to wrap every exporter call and set/reset the context class loader before/after.

Part of this PR brings improvements to the exporter tests, mostly relating to external exporters. It introduces ByteBuddy as a simpler alternative to building external exporters: ByteBuddy is a battle tested library which gives you an easy to use API to implement classes on the fly and package them as JARs. Additionally, by using this instead of pre-made classes and building JARs out of them, we can guarantee that the class ByteBuddy creates does not exist in the current class loader, which increases the robustness of our tests.

This PR also introduces junit5 to the broker and migrates the modified tests to it, as part of our long term goals to migrate from junit4 to junit5.

I recommend the reviewer to check commit-by-commit, but keep in mind that the last commit is the "goal" of the PR, and all the work prior to it is essentially building up to it. I'm also happy to split this into multiple PR if that's easier to review instead, don't hesitate to ask! I hesitated on this as I felt it was important to keep the context, but in the end I think the reviewer will have better insight in what's easier for them.

I have some worries about the PR, in that while using ByteBuddy is probably more reliable than writing out own code to churn out classes on the fly and create JARs for them, it's also a little "arcane" if you've never used it, and I fear the test is maybe more complex than desired. I'd be happy to hear thoughts on how we could avoid this complexity while still testing the functionality. Although working on this really impressed on me that we may want to rethink exporters, and instead of running arbitrary code in the broker, we may just want to provide an API to get data out.

## Related issues

closes #4196 



Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
@ghost ghost closed this as completed in 13ef5d3 Jun 2, 2021
ghost pushed a commit that referenced this issue Jun 3, 2021
7177: [Backports stable/1.0] Fix issues with class loading for external exporters relying on the Thread's context class loader r=npepinpe a=npepinpe

## Description

This PR backports #7150.

## Related issues

backports #7150
relates to #4196



7182: [Backport stable/1.0] Re-enable gocompat and protolock checks r=npepinpe a=github-actions[bot]

# Description
Backport of #7181 to `stable/1.0`.

Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
ghost pushed a commit that referenced this issue Jun 3, 2021
7177: [Backports stable/1.0] Fix issues with class loading for external exporters relying on the Thread's context class loader r=npepinpe a=npepinpe

## Description

This PR backports #7150.

## Related issues

backports #7150
relates to #4196



Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
ghost pushed a commit that referenced this issue Jun 3, 2021
7177: [Backports stable/1.0] Fix issues with class loading for external exporters relying on the Thread's context class loader r=npepinpe a=npepinpe

## Description

This PR backports #7150.

## Related issues

backports #7150
relates to #4196



Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
ghost pushed a commit that referenced this issue Jun 4, 2021
7177: [Backports stable/1.0] Fix issues with class loading for external exporters relying on the Thread's context class loader r=npepinpe a=npepinpe

## Description

This PR backports #7150.

## Related issues

backports #7150
relates to #4196



7182: [Backport stable/1.0] Re-enable gocompat and protolock checks r=npepinpe a=github-actions[bot]

# Description
Backport of #7181 to `stable/1.0`.

Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/low Marks a bug as having little to no noticeable impact for the user
Projects
None yet
1 participant