Use classloaders to load Java functions #4685

jerrypeng · 2019-07-08T00:09:57Z

Master Issue: #4277

Motivation

Currently we use a heavily shaded JAR to run Java functions. There are some disadvantages to this approach

Every time a dependency is added (even for transitive dependencies), we need to remember to to shade it.
Take up extra space (especially relevant for docker images) since we are essentially duplicating classes in the java-instance shaded JAR
Does not achieve total use code isolation as users still cannot run functions that use pulsar-admin-client or pulsar-client

Modifications

Instead of using a shaded JAR to start a java function instance, use different classloaders to load the internal pulsar code, user code, and the interfaces that the two interacts with each other.

key changes:

removed all shading from runtime-all module. Now it just produces a small java-instance jar that contains only the interfaces and logging dependencies.
Changed log4j files from yaml format to xml because using yaml format requires additional dependencies.

Please refer to issue for more details. I can also explain in more detail if the changes are unclear

jerrypeng · 2019-07-08T04:23:02Z

rerun java8 tests

merlimat

Looks good, just a couple of comments

merlimat · 2019-07-08T16:05:21Z

distribution/server/src/assemble/bin.xml

@@ -131,6 +131,8 @@
        <exclude>io.netty:netty-transport-native-epoll</exclude>
        <exclude>io.netty:netty-transport-native-unix-common</exclude>

+        <exclude>org.apache.pulsar:pulsar-functions-runtime-all</exclude>


Couldn't the runtime-all module just be deleted at this point?

The point of having it in the pom is to make sure that the pulsar-functions-runtime-all module is built before this module, which we still need it to

merlimat · 2019-07-08T16:06:17Z

pom.xml

@@ -160,7 +160,8 @@ flexible messaging model and an intuitive client API.</description>
    <rocksdb.version>5.13.3</rocksdb.version>
    <slf4j.version>1.7.25</slf4j.version>
    <commons.collections.version>3.2.2</commons.collections.version>
-    <log4j2.version>2.10.0</log4j2.version>
+    <log4j2.version>2.11.2</log4j2.version>


Is this upgrade required by this PR? In any case, I'd leave it separate

Yes, there is a bug related to classloading in previous version

merlimat · 2019-07-08T16:08:46Z

...ctions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java

        try {
            this.sink.write(new SinkRecord<>(srcRecord, output));
        } catch (Exception e) {
            log.info("Encountered exception in sink write: ", e);
            stats.incrSinkExceptions(e);
            throw new RuntimeException(e);
        }
+        Thread.currentThread().setContextClassLoader(frameworkClassLoader);


This should be done in a finally {} block, otherwise the classloader is not reset after the runtime exception is thrown.

merlimat · 2019-07-08T16:09:54Z

...ctions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java

        if (sourceSpec.getConfigs().isEmpty()) {
            this.source.open(new HashMap<>(), contextImpl);
        } else {
            this.source.open(new Gson().fromJson(sourceSpec.getConfigs(),
                    new TypeToken<Map<String, Object>>(){}.getType()), contextImpl);
        }
+        Thread.currentThread().setContextClassLoader(this.frameworkClassLoader);


Same here, we should use try / finally to make sure it goes back to frameworkClassLoader

…ctor_function_classloading

jerrypeng · 2019-07-09T00:09:46Z

rerun integration tests

merlimat

👍

jerrypeng · 2019-07-09T18:08:19Z

rerun integration tests

jerrypeng · 2019-07-09T19:01:56Z

rerun cpp tests

jerrypeng · 2019-07-09T21:01:25Z

rerun integration tests

jerrypeng · 2019-07-09T21:05:25Z

rerun java8 tests

jerrypeng · 2019-07-09T22:37:46Z

rerun java8 tests

jerrypeng · 2019-07-10T05:55:18Z

rerun java8 tests

sijie · 2019-07-10T07:20:17Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/internal/ReflectionUtils.java

-            return (Class<T>) DefaultImplementation.class.getClassLoader().loadClass(className);
+            try {
+                // when the API is loaded in the same classloader as the impl
+                return (Class<T>) DefaultImplementation.class.getClassLoader().loadClass(className);


Can't we just use Thread.currentThread().getContextClassLoader().loadClass(className); ?

So there can be scenario in which pulsar-client-api and pulsar-client-impl is loaded in a classloader that is NOT the context classloader for whatever reason. For BC purposes lets first check if pulsar-client-impl is loaded in the same classloader as the api and then we can check the context classloader.

This problem was discovered because one of tests in the pulsar-kafka-compat module failed due do powermock loading pulsar-client-api and impl in the powermock classloader and not the context/application classloader.

In theory a user can do that as well.

sijie · 2019-07-10T07:20:32Z

pulsar-client-kafka-compat/pulsar-client-kafka/pom.xml

@@ -62,6 +62,14 @@
        </exclusion>
      </exclusions>
    </dependency>
+    <dependency>


what are the changes for?

These imports were missing for the pulsar-client-kafka module. A test in this module was failing because of a classloader powermock issue but I could not compile this module by itself without these dependencies. Somehow there is no issue if you compile the whole pulsar project.

if so, can you do it in a separated pull request?

sijie · 2019-07-10T07:21:38Z

pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/InstanceUtils.java

@@ -136,4 +138,22 @@ public static String getDefaultSubscriptionName(Function.FunctionDetails functio
        properties.put("instance_id", String.valueOf(instanceId));
        return properties;
    }
+
+    public static boolean isAssignable(Class from, Class to) {


who is using this method?

We can remove this

sijie · 2019-07-10T07:21:53Z

pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstance.java

@@ -22,9 +22,11 @@
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;

+import org.apache.pulsar.functions.api.Context;


what are these two imports for?

was for debugging but not needed anymore

sijie · 2019-07-10T07:22:37Z

...ctions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java

@@ -132,6 +130,9 @@

    private final Map<String, String> properties;

+    private final ClassLoader frameworkClassLoader;


we don't use framework elsewhere. shall we just call it instanceClassLoader or runtimeClassLoader?

we can call it runtimeClassLoader

sijie · 2019-07-10T07:24:07Z

...ctions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java

@@ -425,23 +432,33 @@ private void processResult(Record srcRecord,
    }

    private void sendOutputMessage(Record srcRecord, Object output) {
+        if (!(this.sink instanceof PulsarSink)) {
+            Thread.currentThread().setContextClassLoader(functionClassLoader);


have you benchmarked this? since this is done per message basis. I see this has been used in quite a lot of places.

Yes I have benchmarked setting and getting the context class loader. It takes negligible time (in the order on tens of nano seconds). If you look that the implementation, the code is only setting a pointer or returning a pointer

sijie · 2019-07-10T07:27:23Z

...nctions/runtime-all/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceMain.java

+
+        // Get classpath for framework
+        String framework_classpath = System.getProperty(FUNCTIONS_FRAMEWORK_CLASSPATH);
+        assert framework_classpath != null;


don't use assert. use checkArgument

that uses guava. I don't want to pull in that dependency at this point. This class should have minimum dependencies

if you can't pull in guava, then just throw IllegalArgument runtime exception if framework_class is not. because assert doesn't always work.

sijie · 2019-07-10T07:27:45Z

...nctions/runtime-all/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceMain.java

+ */
+public class JavaInstanceMain {
+
+    private static final String FUNCTIONS_FRAMEWORK_CLASSPATH = "pulsar.functions.framework.classpath";


change "framework"?

…/incubator-pulsar into refactor_function_classloading

jerrypeng · 2019-07-11T22:50:21Z

@sijie thanks for the review. PTAL

jerrypeng · 2019-07-12T16:40:36Z

rerun integration tests

* Use classloading to load use code for functions

jiazhai · 2019-08-28T07:28:33Z

conflict with #5046, mark as 2.4.1

* Use classloading to load use code for functions (cherry picked from commit 6ff1bba)

jerrypeng added 8 commits July 2, 2019 12:05

Use classloading to load use code for functions

94f7907

improving impl

20397b4

adding to impl

4d6f30c

cleaning up

ae39881

fixing minor issues

418c25a

fixing bash script

9e097a3

reverting configs

22383a0

cleaning up

f54dcbb

jerrypeng added the area/function label Jul 8, 2019

jerrypeng added this to the 2.5.0 milestone Jul 8, 2019

jerrypeng requested review from merlimat, sijie, srkukarni and rdhabalia July 8, 2019 00:09

jerrypeng self-assigned this Jul 8, 2019

fixing tests

f48b4a4

fix distribution packaging

6f9903d

merlimat reviewed Jul 8, 2019

View reviewed changes

jerrypeng added 3 commits July 8, 2019 10:10

addressing comments

0df81e6

Merge branch 'master' of github.com:apache/incubator-pulsar into refa…

957d6db

…ctor_function_classloading

fix bug

c3c2481

jerrypeng added 4 commits July 8, 2019 23:11

revert log4j version change

d403a47

remove newline

d805bef

updating pom

72a36f1

adding missing license headers

3764447

merlimat approved these changes Jul 9, 2019

View reviewed changes

Merge branch 'master' into refactor_function_classloading

275c985

sijie reviewed Jul 10, 2019

View reviewed changes

jerrypeng added 5 commits July 11, 2019 15:12

addressing comments

4b0f915

Merge branch 'refactor_function_classloading' of github.com:jerrypeng…

bbbb162

…/incubator-pulsar into refactor_function_classloading

cleaning up

9dbcc6d

renaming to instance classloader

41be88d

renaming some more

1d7ae29

addressing comments

5d22673

fix bug

5b18871

jerrypeng merged commit 6ff1bba into apache:master Jul 13, 2019

easyfan pushed a commit to easyfan/pulsar that referenced this pull request Jul 26, 2019

Use classloaders to load Java functions (apache#4685)

8666581

* Use classloading to load use code for functions

jiazhai modified the milestones: 2.5.0, 2.4.1 Aug 28, 2019

jiazhai pushed a commit that referenced this pull request Aug 28, 2019

Use classloaders to load Java functions (#4685)

52af202

* Use classloading to load use code for functions (cherry picked from commit 6ff1bba)

jerrypeng mentioned this pull request Sep 4, 2019

Fix: windowed functions were broken when we changed java function instance to use classloaders #5117

Merged

		@@ -132,6 +130,9 @@

		private final Map<String, String> properties;

		private final ClassLoader frameworkClassLoader;

Use classloaders to load Java functions #4685

Use classloaders to load Java functions #4685

Conversation

jerrypeng commented Jul 8, 2019 • edited

Motivation

Modifications

jerrypeng commented Jul 8, 2019

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng Jul 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Jul 9, 2019

merlimat left a comment

Choose a reason for hiding this comment

jerrypeng commented Jul 9, 2019

jerrypeng commented Jul 9, 2019

jerrypeng commented Jul 9, 2019

jerrypeng commented Jul 9, 2019

jerrypeng commented Jul 9, 2019

jerrypeng commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Jul 11, 2019

jerrypeng commented Jul 12, 2019

jiazhai commented Aug 28, 2019

jerrypeng commented Jul 8, 2019 •

edited

jerrypeng Jul 8, 2019 •

edited