You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/08/30 08:41:55 UTC
[GitHub] [flink] lsyldliu opened a new pull request, #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
lsyldliu opened a new pull request, #20713:
URL: https://github.com/apache/flink/pull/20713
## What is the purpose of the change
When enabling checkpoint, if using scalar UDF, the UDF class will be loaded during generating JobGraph. But the UDF class is in the user classloader, we can't load it from thread context classloader, so we should use the user classloader to compile JobGraph when needed.
## Brief change log
Fix the bug of failing to generate Jobgraph when using scalar udf and enabling checkpoint
## Verifying this change
This change added tests and can be verified as follows:
*(example:)*
- *Added e2e tests in UsingRemoteJarITCase*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1233670182
cc @zhuzhurk @wuchong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1237021450
@lsyldliu , could you open another PR for release-1.16 branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on a diff in pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on code in PR #20713:
URL: https://github.com/apache/flink/pull/20713#discussion_r960631789
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java:
##########
@@ -293,7 +294,8 @@ public StreamExecutionEnvironment(
this.configure(this.configuration, this.userClassloader);
}
- protected ClassLoader getUserClassloader() {
+ @VisibleForTesting
+ public ClassLoader getUserClassloader() {
Review Comment:
What's your concern about exposing this method for tests? Can you give me some advice about what classloader we should use? `env.getClass().getClassLoader()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1234261639
@zhuzhurk Thanks for review, I've updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #20713:
URL: https://github.com/apache/flink/pull/20713#discussion_r960302329
##########
flink-end-to-end-tests/flink-end-to-end-tests-sql/src/test/java/org/apache/flink/table/sql/codegen/UsingRemoteJarITCase.java:
##########
@@ -102,14 +103,24 @@ public void destroyHDFS() {
@Test
public void testUdfInRemoteJar() throws Exception {
runAndCheckSQL(
- "remote_jar_e2e.sql",
+ "remote_udf_e2e.sql",
Review Comment:
Seems `remote_udf_e2e.sql` does not exist?
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamGraph.java:
##########
@@ -1008,12 +1008,12 @@ private void removeVertex(StreamNode toRemove) {
/** Gets the assembled {@link JobGraph} with a random {@link JobID}. */
public JobGraph getJobGraph() {
Review Comment:
Let's annotate this method with `@VisibleForTesting` because it is used only for tests.
This somehow means it's safe to use the thread classloader.
##########
flink-clients/src/main/java/org/apache/flink/client/StreamGraphTranslator.java:
##########
@@ -40,14 +40,20 @@ public class StreamGraphTranslator implements FlinkPipelineTranslator {
private static final Logger LOG = LoggerFactory.getLogger(StreamGraphTranslator.class);
+ private final ClassLoader userClassloader;
+
+ public StreamGraphTranslator(ClassLoader userClassloader) {
+ this.userClassloader = userClassloader;
+ }
+
@Override
public JobGraph translateToJobGraph(
Review Comment:
Looks to me it's better to pass in `userClassloader` as a parameter of this method, instead of making it a field of `StreamGraphTranslator.java`. In this way, we no longer need to change `FlinkPipelineTranslationUtil#translateToJSONExecutionPlan()` and its usages.
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java:
##########
@@ -293,7 +294,8 @@ public StreamExecutionEnvironment(
this.configure(this.configuration, this.userClassloader);
}
- protected ClassLoader getUserClassloader() {
+ @VisibleForTesting
+ public ClassLoader getUserClassloader() {
Review Comment:
We should not make this method public because `StreamExecutionEnvironment` is `@Public`, especially given that this change is for test only.
##########
flink-clients/src/main/java/org/apache/flink/client/deployment/executors/PipelineExecutorUtils.java:
##########
@@ -42,10 +42,13 @@ public class PipelineExecutorUtils {
* @param configuration the configuration with the necessary information such as jars and
* classpaths to be included, the parallelism of the job and potential savepoint settings
* used to bootstrap its state.
+ * @param userClassloader the classloader used to load class during compile JobGraph
Review Comment:
maybe "the classloader which can load user classes", I think it's more accurate because currently it's the system classloader which is mainly used to build JobGraph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong merged pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
wuchong merged PR #20713:
URL: https://github.com/apache/flink/pull/20713
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on a diff in pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on code in PR #20713:
URL: https://github.com/apache/flink/pull/20713#discussion_r962452744
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java:
##########
@@ -293,7 +294,8 @@ public StreamExecutionEnvironment(
this.configure(this.configuration, this.userClassloader);
}
- protected ClassLoader getUserClassloader() {
+ @VisibleForTesting
+ public ClassLoader getUserClassloader() {
Review Comment:
The ci of runtime has passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1237022592
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1237886529
> * PENDING
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1231359579
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "91609148f6eede8a42aeae3e27ece77d82ab8d3b",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "91609148f6eede8a42aeae3e27ece77d82ab8d3b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 91609148f6eede8a42aeae3e27ece77d82ab8d3b UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #20713:
URL: https://github.com/apache/flink/pull/20713#discussion_r962445460
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/StreamExecutionEnvironment.java:
##########
@@ -293,7 +294,8 @@ public StreamExecutionEnvironment(
this.configure(this.configuration, this.userClassloader);
}
- protected ClassLoader getUserClassloader() {
+ @VisibleForTesting
+ public ClassLoader getUserClassloader() {
Review Comment:
When it is public, users will also have access to it, which is what we want.
Will these test pass if we pass the default classloader in as the user classloader?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20713: [FLINK-28897][runtime] Fix bug of failing to generate Jobgraph when using scalar udf and enable checkpoint
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20713:
URL: https://github.com/apache/flink/pull/20713#issuecomment-1234957259
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org