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

Shade pulsar-functions-runtime instead of pulsar-functions-worker #1351

Merged
merged 5 commits into from
Mar 7, 2018

Conversation

sijie
Copy link
Member

@sijie sijie commented Mar 6, 2018

Motivation

pulsar-functions-runtime is required by worker/broker and also it would be packaged into a fat jar to used by process runtime to invoke function executions.

so

  1. correct shading point is at pulsar-functions-runtime not pulsar-functions-worker.
  2. we need to shade protobuf to ensure the protobuf used by bk/dlog/functions is not conflicting with the protobuf used by client/broker.
  3. pulsar is using a pretty new netty jar (4.1.21), which has some breaking changes at `io.netty.util.internal.ReflectionUtil'. it can't work with other older version. so we need to shade netty.

Modifications

  • runtime-shade: package all the dependencies that use netty and protobuf and shade netty and protobuf, it also exclude the shaded client dependency. so that it can be integrated into worker and be used as part of worker.
  • runtime-all: this is the module for packaging runtime-shade and a non-shade pulsar-client to produce a fat jar for java process runtime.
  • change pulsar-client-tools and pulsar-broker to use pulsar-functions-worker since worker module now is using a shaded runtime jar, there will be no protobuf/netty conflicts.

At the same time, changed using bookkeeper/test-jar to bookkeeper-server-tests-shaded, so it won't pollute the classpath.

Result

  • verified on mac, pulsar-functions can run with process/thread mode at broker. all functionalities work.
  • verified on linux, pulsar-functions can run with process/thread mode at broker. all functionalities work.

@sijie sijie requested review from merlimat and rdhabalia March 6, 2018 23:56
@sijie sijie self-assigned this Mar 6, 2018
@sijie
Copy link
Member Author

sijie commented Mar 6, 2018

/cc @srkukarni @jerrypeng

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

<groupId>${project.groupId}</groupId>
<artifactId>pulsar-common</artifactId>
<version>${project.version}</version>
<exclusions>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to exclude checksum here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because bookkeeper-server-shaded jar include the circe-checksum when shading guava. so circe-checksum is effectively available in pulsar-broker when including bookkeeper-server-shaded jar. so excluding pulsar-common to make the behavior more deterministic.

that said, we probably don't have to exclude it though. because in the classpath we have both shaded guava and non-shaded guava.

@sijie
Copy link
Member Author

sijie commented Mar 7, 2018

merged to latest master since the new bookkeeper-server-tests-shaded is already published to apache snapshot.

@sijie sijie merged commit 0ac6b9a into apache:master Mar 7, 2018
@sijie sijie deleted the shade_runtime_instead_of_worker branch March 7, 2018 05:56
lhotari added a commit to lhotari/pulsar that referenced this pull request Feb 19, 2021
- pulsar-functions-worker-shaded was removed already by apache#1351
merlimat pushed a commit that referenced this pull request Feb 20, 2021
- pulsar-functions-worker-shaded was removed already by #1351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants