Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation #9413

jerrypeng · 2021-02-02T00:42:45Z

Motivation

Currently, when a user submits a built-in source or sink to be run, the locally stored NAR file of the built-in source and sink is unpacked or MD5 checksums calculated and loaded into a classloader up to 5 times during the startup and execution process. This is redundant and unnecessary waste of resources. We should just unpack one and cache the results. I have noticed multi-second instance start up latencies because of this.

Modifications

Refactored the code for function instance startup
The initial scan for connectors also caches classloaders, connector definitions, etc so that they can be reused later on
Fix a bug involving updating certain fields (nested) in for built-in Sources and Sinks that get incorrectly reject by worker claiming there is no change.

Verifying this change

Add multiple tests for built-in sources and sinks

sijie · 2021-02-02T01:22:46Z

@freeznet @nlu90 Can you review this pull request?

jerrypeng · 2021-02-02T07:42:35Z

/pulsarbot run-failure-checks

eolivelli

great work !
I left some minor comments

eolivelli · 2021-02-02T10:04:52Z

pulsar-broker/src/test/java/org/apache/pulsar/functions/worker/PulsarFunctionLocalRunTest.java


-    @Test
+    @Test(timeOut = 20000)


I am not sure we need these timeouts,
if the test breaks then it is not likely that other tests will run.

The individual timeout is in place so we don't just block for 1.5-2 hour which is the absolute test timeout and consume resources for that time.

We centrally set 300 seconds timeouts for all the tests that don't have a timeout

eolivelli · 2021-02-02T10:05:33Z

pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java

@@ -354,6 +377,8 @@ private PulsarWorkerService createPulsarFunctionWorker(ServiceConfiguration conf
        workerConfig.setAuthenticationEnabled(true);
        workerConfig.setAuthorizationEnabled(true);

+        workerConfig.setConnectorsDirectory(Files.createTempDirectory("test").toFile().getAbsolutePath());


what about naming the directory "tempconnectorsdir" ?

eolivelli · 2021-02-02T10:07:45Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java

            try {
-                sinkClassName = ConnectorUtils.getIOSinkClass(narClassLoader);
+                sinkClassName = ConnectorUtils.getIOSinkClass((NarClassLoader) sinkClassLoader);


if we are blindly casting to NarClassLoader can we change the type of the argument ?

sinkClassLoader may not be an instance of NarClassLoader. In the line of code, if the sinkClassName is not specified, we are only trying to see if we can find a className specified in the NAR.

david-streamlio · 2021-02-02T14:55:55Z

LGTM

nlu90

LGTM except some minor comments

nlu90 · 2021-02-04T17:55:46Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/io/ConnectorUtils.java

-    public static List<ConfigFieldDefinition> getConnectorConfigDefinition(String narPath,
-                                                                           String configClassName,
-                                                                           String narExtractionDirectory) throws Exception {
+    public static List<ConfigFieldDefinition> getConnectorConfigDefinition(ClassLoader narClassLoader,


Do you mean NarClassLoader narClassLoader here?

We don't need a NarClassLoader. A ClassLoader should suffice. I will rename the variable

nlu90 · 2021-02-04T18:06:22Z

pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionActioner.java

-            throws IOException, ClassNotFoundException {
-        try (NarClassLoader ncl = NarClassLoader.getFromArchive(archive, Collections.emptySet(), workerConfig.getNarExtractionDirectory())) {
-            String typeArg = getSourceType(className, ncl).getName();
+    private void fillSourceTypeClass(FunctionDetails.Builder functionDetails,


this method and the following fillSinkTypeClass method looks very similar, wondering if we could implement them into one single method to reduce code duplication.

Not sure how much code we can save here because fillSourceTypeClass() sets the typeClassName for the source and checks if a sinkSpec exists and set the typeClassName for that as well. The logic for fillSinkTypeClass() is the reverse.

Also, since this refactor is already large, I don't want to make more changes in the code. If we want to refactor more, let's do it in a subsequent PR.

nlu90 · 2021-02-04T18:12:38Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java

+            }
+
+        } else {
+            // if connector class name is provided, we need to try to load it as a JAR and as a NAR.


nlu90 · 2021-02-04T18:21:31Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java

+            File packageFile,
+            String narExtractionDirectory) {
+        String connectorClassName = className;
+        ClassLoader classLoader;


instead of having this variable and return it at the end. I suggest return the classLoader at the end of each if/else clause. Easier to read

nlu90 · 2021-02-04T18:22:46Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java

+                if (jarClassLoaderException != null) {
+                    errorMsg.append("Attempts to load it as a JAR package produced error: " + jarClassLoaderException.getMessage());
+                }
+


do you want to add some linebreak or delimiter between these two error message for better reading?

I can add a space

jerrypeng · 2021-02-04T20:40:20Z

/pulsarbot run-failure-checks

…packing and checksum calculation

…packing and checksum calculation (apache#9413) Co-authored-by: Jerry Peng <jerryp@splunk.com>

…y usage - Pulsar loads all nar files to memory after apache/pulsar#9413

lhotari · 2021-11-25T08:16:11Z

I reported an issue #12974 since Functions worker now consume more resident memory after these changes.

* Reduce CI memory usage * Limit MaxDirectMemorySize to 256m on bookkeeper, broker and function worker * Reduce limits even further because of 143 exit code * Add logging to loading of images to kind * Use single worker node for CI kind cluster * Use datastax/lunastreaming image for functions worker to reduce memory usage - Pulsar loads all nar files to memory after apache/pulsar#9413 * Add solution for skipping yq installation (for local development) * Bump k8s version used in kind * Add instructions how to run locally * Allow less diskspace for CI test run

jerrypeng added area/function area/connector labels Feb 2, 2021

jerrypeng added this to the 2.8.0 milestone Feb 2, 2021

jerrypeng self-assigned this Feb 2, 2021

jerrypeng changed the title ~~Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation~~ WIP Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation Feb 2, 2021

jerrypeng changed the title ~~WIP Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation~~ Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation Feb 2, 2021

jerrypeng requested review from srkukarni, sijie and merlimat February 2, 2021 03:01

eolivelli reviewed Feb 2, 2021

View reviewed changes

merlimat approved these changes Feb 3, 2021

View reviewed changes

freeznet approved these changes Feb 4, 2021

View reviewed changes

nlu90 approved these changes Feb 4, 2021

View reviewed changes

Jerry Peng added 7 commits February 4, 2021 15:43

Optimize built-in source/sink startup by eliminating redundant NAR un…

4f6ba22

…packing and checksum calculation

adding mising file

97f15bb

cleaning up

fc6282a

fix tests

7b5fb99

cleaning up

c458df6

change temp test connector folder name

c9b4ca5

addressing comments

a5b7f41

jerrypeng force-pushed the optimize_builtin_connector_startup branch from dd2082a to a5b7f41 Compare February 4, 2021 23:45

jerrypeng merged commit 1ff1fcd into apache:master Feb 5, 2021

jerrypeng mentioned this pull request Feb 6, 2021

Optimize built-in source/sink startup Part 2 #9500

Merged

merlimat pushed a commit to merlimat/pulsar that referenced this pull request Apr 6, 2021

Optimize built-in source/sink startup by eliminating redundant NAR un…

ec5c598

…packing and checksum calculation (apache#9413) Co-authored-by: Jerry Peng <jerryp@splunk.com>

lhotari added a commit to lhotari/datastax-pulsar-helm-chart that referenced this pull request Nov 25, 2021

Use datastax/lunastreaming image for functions worker to reduce memor…

860708f

…y usage - Pulsar loads all nar files to memory after apache/pulsar#9413

lhotari mentioned this pull request Nov 25, 2021

[Functions] Function workers require more memory on Pulsar 2.8.x+ #12974

Open

sijie mentioned this pull request Nov 25, 2021

ISSUE-12974: [Functions] Function workers require more memory on Pulsar 2.8.x+ streamnative/pulsar-archived#3322

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation #9413

Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation #9413

jerrypeng commented Feb 2, 2021

sijie commented Feb 2, 2021

jerrypeng commented Feb 2, 2021

eolivelli left a comment

eolivelli Feb 2, 2021

jerrypeng Feb 2, 2021

merlimat Feb 3, 2021

eolivelli Feb 2, 2021

jerrypeng Feb 2, 2021

eolivelli Feb 2, 2021

jerrypeng Feb 2, 2021

david-streamlio commented Feb 2, 2021

nlu90 left a comment

nlu90 Feb 4, 2021

jerrypeng Feb 4, 2021

nlu90 Feb 4, 2021

jerrypeng Feb 4, 2021

nlu90 Feb 4, 2021

nlu90 Feb 4, 2021

jerrypeng Feb 4, 2021

nlu90 Feb 4, 2021

jerrypeng Feb 4, 2021

jerrypeng commented Feb 4, 2021

lhotari commented Nov 25, 2021

Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation #9413

Optimize built-in source/sink startup by eliminating redundant NAR unpacking and checksum calculation #9413

Conversation

jerrypeng commented Feb 2, 2021

Motivation

Modifications

Verifying this change

sijie commented Feb 2, 2021

jerrypeng commented Feb 2, 2021

eolivelli 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

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

david-streamlio commented Feb 2, 2021

nlu90 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

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 Feb 4, 2021

lhotari commented Nov 25, 2021