-
Notifications
You must be signed in to change notification settings - Fork 6
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
[improve][fn] Cherry-picked commit Optimize Function Worker startup apache#22122 #228
Conversation
…rect zip/bytecode access (apache#22122) (cherry picked from commit bbc6224) (cherry picked from commit 597cfa1) (cherry picked from commit d608bfc) Conflicts: pulsar-broker/pom.xml pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/thread/ThreadRuntime.java pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/thread/ThreadRuntimeFactory.java pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/FunctionsManager.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/functions/FunctionArchive.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/functions/FunctionUtils.java pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/io/ConnectorUtils.java pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/FunctionCommonTest.java pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/FunctionConfigUtilsTest.java pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java pulsar-functions/worker/pom.xml pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SinksImpl.java pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourcesImpl.java pulsar-functions/worker/src/test/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImplTest.java pulsar-functions/worker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/FunctionApiV2ResourceTest.java pulsar-functions/worker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v3/FunctionApiV3ResourceTest.java pulsar-functions/worker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v3/SinkApiV3ResourceTest.java
@@ -119,24 +118,24 @@ private static ClassLoader getFunctionClassLoader(InstanceConfig instanceConfig, | |||
String narExtractionDirectory, | |||
FunctionCacheManager fnCache, | |||
Optional<ConnectorsManager> connectorsManager, | |||
Optional<FunctionsManager> functionsManager, | |||
Function.FunctionDetails.ComponentType componentType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoloboschi , On apache 2_10 component type is initialised in the function and not using the argument.
Can you have a look and let me know if its okay to remove the componentType parameter from the function.
The 2 places where this function getFunctionClassLoader is called are -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as it comes from the same logic, it's okay to drop the function arg
@@ -399,8 +404,7 @@ public static SinkConfig convertFromDetails(FunctionDetails functionDetails) { | |||
} | |||
|
|||
public static ExtractedSinkDetails validateAndExtractDetails(SinkConfig sinkConfig, | |||
ClassLoader sinkClassLoader, | |||
ClassLoader functionClassLoader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoloboschi , In apache we are not validating functionClassLoader,
FunctionClassLoader and functionClass name is used to resolve typeArg and inputClassLoader, but in apache, we are using typeArg is defined and we are using inputFunction.getTypePool() instead of inputClassLoader at the validation points.
Can you have a look if removing the param breaks any functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This difference comes from the fact that LS 2.10 has PIP-193 in it while AP 2.10 hasn't (it's only available since 3.0.0)
We should apply the same fix to both the functions (both the sink and the pre-processing)
In this case the sinkClassLoader
is the sink and functionClassLoader
is the preprocessing function
Motivation
Cherry-picking commit from Apache for optimizing functions worker
Master Issue: apache#22122
Verifying this change
(Please pick either of the following options)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?
doc-required
(If you need help on updating docs, create a doc issue)
no-need-doc
(Please explain why)
doc
(If this PR contains doc changes)