azagrebin commented on a change in pull request #12446:
URL: https://github.com/apache/flink/pull/12446#discussion_r437280460
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheRecoveryITCase.java
##########
@@ -79,7 +79,9 @@ public void testRecoveryRegisterAndDownload() throws
Exception {
final BlobLibraryCacheManager.ClassLoaderFactory
classLoaderFactory = BlobLibraryCacheManager.defaultClassLoaderFactory(
FlinkUserCodeClassLoaders.ResolveOrder.CHILD_FIRST,
- new String[0]);
+ new String[0],
+ false,
+ exception -> {});
Review comment:
true, we can just make it nullable
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java
##########
@@ -151,12 +163,31 @@ public URLClassLoader createClassLoader(URL[]
libraryURLs) {
classLoaderResolveOrder,
libraryURLs,
FlinkUserCodeClassLoaders.class.getClassLoader(),
- alwaysParentFirstPatterns);
+ alwaysParentFirstPatterns,
+ classLoadingExceptionHandler);
}
}
- public static ClassLoaderFactory
defaultClassLoaderFactory(FlinkUserCodeClassLoaders.ResolveOrder
classLoaderResolveOrder, String[] alwaysParentFirstPatterns) {
- return new DefaultClassLoaderFactory(classLoaderResolveOrder,
alwaysParentFirstPatterns);
+ public static ClassLoaderFactory defaultClassLoaderFactory(
+ FlinkUserCodeClassLoaders.ResolveOrder
classLoaderResolveOrder,
+ String[] alwaysParentFirstPatterns,
+ boolean failOnJvmMetaspaceOomError,
+ FatalErrorHandler fatalErrorHandler) {
+ return new DefaultClassLoaderFactory(
+ classLoaderResolveOrder,
+ alwaysParentFirstPatterns,
+
createClassLoadingExceptionHandler(failOnJvmMetaspaceOomError,
fatalErrorHandler));
+ }
+
+ private static Consumer<Throwable> createClassLoadingExceptionHandler(
Review comment:
true but the call will have to be duplicated for current use cases and I
am not sure about any other at the moment. I think we can achieve the same by
making the `fatalErrorHandler` nullable.
##########
File path:
flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java
##########
@@ -110,6 +110,14 @@
" resolved through the parent ClassLoader first. A
pattern is a simple prefix that is checked against" +
" the fully qualified class name. These patterns are
appended to \"" + ALWAYS_PARENT_FIRST_LOADER_PATTERNS.key() + "\".");
+ @Documentation.Section(Documentation.Sections.EXPERT_CLASS_LOADING)
+ public static final ConfigOption<Boolean>
FAIL_USER_CLASS_LOADING_METASPACE_OOM = ConfigOptions
+ .key("classloader.fail.on.metaspace.oom.error")
+ .booleanType()
+ .defaultValue(true)
+ .withDescription("Sets whether to fail and exit TaskManager JVM
process if 'OutOfMemoryError: Metaspace' is " +
+ "thrown trying to load a user code class.");
Review comment:
I would add then `user or plugin` because if we drop `user` at all, it
will be confusing. The option is really a fallback only for user scope class
loading, not for all places where this exception can be thrown.
I would keep the internal variable name as it is because this is how we call
the respective classes it tweaks at the moment: `FlinkUserCodeClassLoader`.
----------------------------------------------------------------
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]
