rmetzger commented on a change in pull request #13355:
URL: https://github.com/apache/flink/pull/13355#discussion_r487783536
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/operators/chaining/ChainedDriver.java
##########
@@ -63,8 +64,8 @@
public void setup(TaskConfig config, String taskName, Collector<OT>
outputCollector,
- AbstractInvokable parent, ClassLoader
userCodeClassLoader, ExecutionConfig executionConfig,
- Map<String, Accumulator<?,?>> accumulatorMap)
+ AbstractInvokable parent,
UserCodeClassLoader userCodeClassLoader, ExecutionConfig executionConfig,
Review comment:
Note: We need to forward the `UserCodeClassLoader`, so that we can pass
it into the `DistributedRuntimeUDFContext`.
But I was able reduce the `userCodeClassLoader` field to `ClassLoader` again.
##########
File path: flink-connectors/flink-connector-cassandra/pom.xml
##########
@@ -187,6 +187,13 @@ under the License.
<scope>test</scope>
<type>test-jar</type>
</dependency>
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-core</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
Review comment:
I added this dependency because the `TestingUserCodeClassLoader` is in
`flink-core`. However looking at the usages of that class, it can be in
`flink-runtime` as well. I should have done that check before changing all the
POM files 🙈
##########
File path:
flink-connectors/flink-connector-kinesis/src/test/resources/log4j2-test.properties
##########
@@ -0,0 +1,29 @@
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+# Set root logger level to OFF to not flood build logs
+# set manually to INFO for debugging purposes
+rootLogger.level = OFF
Review comment:
No, but this module was lacking the logging files, and I needed it for
testing.
I don't think there's harm in adding this file?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMaster.java
##########
@@ -157,7 +158,7 @@
private final FatalErrorHandler fatalErrorHandler;
- private final ClassLoader userCodeLoader;
+ private final UserCodeClassLoader userCodeLoader;
Review comment:
You are right, we could revert these changes and call the
`.asClassloader()` method when passing the classloader into the factory.
I'm not aware of plans to use the UserCodeCLassloader in JM hooks. (We could
pass it into the InitializeOnMaster.initializeGlobal() interface, but that
would break a public interface)
I also could not find a strong argument for passing the
`UserCodeClassLoader` into the `JobMaster`. I guess this is a question of
taste, rather than strong logical arguments.
The benefit of the current approach is that we have an extra information
about the type of ClassLoader we are using.
I'm undecided here. If you believe it is better to revert this changes, I
can do 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.
For queries about this service, please contact Infrastructure at:
[email protected]
