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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
