From 6bf6c6478f7cfbf392eb2cc965651936af2fc606 Mon Sep 17 00:00:00 2001 From: zentol Date: Wed, 21 Feb 2018 14:55:22 +0100 Subject: [PATCH 1/3] [FLINK-8645][configuration] Split classloader.parent-first-patterns into "base" and "append --- docs/ops/config.md | 12 +++-- .../flink/configuration/CoreOptions.java | 35 +++++++++++- .../flink/configuration/CoreOptionsTest.java | 54 +++++++++++++++++++ .../jobmaster/JobManagerSharedServices.java | 4 +- .../TaskManagerConfiguration.java | 4 +- .../flink/runtime/jobmanager/JobManager.scala | 5 +- 6 files changed, 99 insertions(+), 15 deletions(-) create mode 100644 flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java diff --git a/docs/ops/config.md b/docs/ops/config.md index efd3ee3af9037..f249a275f2cba 100644 --- a/docs/ops/config.md +++ b/docs/ops/config.md @@ -75,12 +75,16 @@ without explicit scheme definition, such as `/user/USERNAME/in.txt`, is going to - `classloader.resolve-order`: Whether Flink should use a child-first `ClassLoader` when loading user-code classes or a parent-first `ClassLoader`. Can be one of `parent-first` or `child-first`. (default: `child-first`) -- `classloader.parent-first-patterns`: A (semicolon-separated) list of patterns that specifies which +- `classloader.parent-first-patterns.base`: A (semicolon-separated) list of patterns that specifies which classes should always be resolved through the parent `ClassLoader` first. A pattern is a simple prefix that is checked against the fully qualified class name. By default, this is set to -`java.;org.apache.flink.;javax.annotation;org.slf4j;org.apache.log4j;org.apache.logging.log4j;ch.qos.logback`. -If you want to change this setting you have to make sure to also include the default patterns in -your list of patterns if you want to keep that default behaviour. +`"java.;scala.;org.apache.flink.;com.esotericsoftware.kryo;org.apache.hadoop.;javax.annotation.;org.slf4j;org.apache.log4j;org.apache.logging.log4j;ch.qos.logback"`. +To extend this list beyond the default it is recommended to configure `classloader.parent-first-patterns.append` instead of modifying this setting directly. + +- `classloader.parent-first-patterns.append`: A (semicolon-separated) list of patterns that specifies which +classes should always be resolved through the parent `ClassLoader` first. A pattern is a simple +prefix that is checked against the fully qualified class name. +This list is appended to `classloader.parent-first-patterns.base`. ## Advanced Options diff --git a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java index 9bd7fab0ee492..3e223f9879520 100644 --- a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java +++ b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java @@ -86,11 +86,42 @@ public class CoreOptions { * */ public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER = ConfigOptions - .key("classloader.parent-first-patterns") + .key("classloader.parent-first-patterns.base") .defaultValue("java.;scala.;org.apache.flink.;com.esotericsoftware.kryo;org.apache.hadoop.;javax.annotation.;org.slf4j;org.apache.log4j;org.apache.logging.log4j;ch.qos.logback") + .withDeprecatedKeys("classloader.parent-first-patterns") .withDescription("A (semicolon-separated) list of patterns that specifies which classes should always be" + " resolved through the parent ClassLoader first. A pattern is a simple prefix that is checked against" + - " the fully qualified class name."); + " the fully qualified class name. This setting should generally not be modified. To add another pattern we" + + " recommend to use \"classloader.parent-first-patterns.append\" instead."); + + public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER_APPEND = ConfigOptions + .key("classloader.parent-first-patterns.append") + .defaultValue("") + .withDescription("A (semicolon-separated) list of patterns that specifies which classes should always be" + + " 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.key() + "\"."); + + private static final String[] EMPTY_STRING_ARRAY = new String[0]; + + public static String[] getParentFirstLoaderPatterns(Configuration config) { + String base = config.getString(ALWAYS_PARENT_FIRST_LOADER); + String append = config.getString(ALWAYS_PARENT_FIRST_LOADER_APPEND); + + String[] basePatterns = base.isEmpty() + ? EMPTY_STRING_ARRAY + : base.split(";"); + + if (append.isEmpty()) { + return basePatterns; + } else { + String[] appendPatterns = append.split(";"); + + String[] joinedPatterns = new String[basePatterns.length + appendPatterns.length]; + System.arraycopy(basePatterns, 0, joinedPatterns, 0, basePatterns.length); + System.arraycopy(appendPatterns, 0, joinedPatterns, basePatterns.length, appendPatterns.length); + return joinedPatterns; + } + } // ------------------------------------------------------------------------ // process parameters diff --git a/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java b/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java new file mode 100644 index 0000000000000..f71fd827f3bdb --- /dev/null +++ b/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java @@ -0,0 +1,54 @@ +/* + * 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. + */ + +package org.apache.flink.configuration; + +import org.junit.Assert; +import org.junit.Test; + +/** + * Tests for {@link CoreOptions}. + */ +public class CoreOptionsTest { + @Test + public void testGetParentFirstLoaderPatterns() { + Configuration config = new Configuration(); + + Assert.assertArrayEquals( + CoreOptions.ALWAYS_PARENT_FIRST_LOADER.defaultValue().split(";"), + CoreOptions.getParentFirstLoaderPatterns(config)); + + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER, "hello;world"); + + Assert.assertArrayEquals( + "hello;world".split(";"), + CoreOptions.getParentFirstLoaderPatterns(config)); + + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_APPEND, "how;are;you"); + + Assert.assertArrayEquals( + "hello;world;how;are;you".split(";"), + CoreOptions.getParentFirstLoaderPatterns(config)); + + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER, ""); + + Assert.assertArrayEquals( + "how;are;you".split(";"), + CoreOptions.getParentFirstLoaderPatterns(config)); + } +} diff --git a/flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerSharedServices.java b/flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerSharedServices.java index 34b338e897375..c1e910cbeb84b 100644 --- a/flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerSharedServices.java +++ b/flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerSharedServices.java @@ -131,9 +131,7 @@ public static JobManagerSharedServices fromConfiguration( final String classLoaderResolveOrder = config.getString(CoreOptions.CLASSLOADER_RESOLVE_ORDER); - final String alwaysParentFirstLoaderString = - config.getString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER); - final String[] alwaysParentFirstLoaderPatterns = alwaysParentFirstLoaderString.split(";"); + final String[] alwaysParentFirstLoaderPatterns = CoreOptions.getParentFirstLoaderPatterns(config); final BlobLibraryCacheManager libraryCacheManager = new BlobLibraryCacheManager( diff --git a/flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerConfiguration.java b/flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerConfiguration.java index aebefd65b0520..cb6fe51b0b900 100644 --- a/flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerConfiguration.java +++ b/flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerConfiguration.java @@ -241,9 +241,7 @@ public static TaskManagerConfiguration fromConfiguration(Configuration configura final String classLoaderResolveOrder = configuration.getString(CoreOptions.CLASSLOADER_RESOLVE_ORDER); - final String alwaysParentFirstLoaderString = - configuration.getString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER); - final String[] alwaysParentFirstLoaderPatterns = alwaysParentFirstLoaderString.split(";"); + final String[] alwaysParentFirstLoaderPatterns = CoreOptions.getParentFirstLoaderPatterns(configuration); final String taskManagerLogPath = configuration.getString(ConfigConstants.TASK_MANAGER_LOG_PATH_KEY, System.getProperty("log.file")); final String taskManagerStdoutPath; diff --git a/flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala b/flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala index d0e401cea5db4..b8523695d29a4 100644 --- a/flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala +++ b/flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala @@ -2430,9 +2430,8 @@ object JobManager { val timeout: FiniteDuration = AkkaUtils.getTimeout(configuration) val classLoaderResolveOrder = configuration.getString(CoreOptions.CLASSLOADER_RESOLVE_ORDER) - val alwaysParentFirstLoaderString = - configuration.getString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER) - val alwaysParentFirstLoaderPatterns = alwaysParentFirstLoaderString.split(';') + + val alwaysParentFirstLoaderPatterns = CoreOptions.getParentFirstLoaderPatterns(configuration) val restartStrategy = RestartStrategyFactory.createRestartStrategyFactory(configuration) From 2d96635afd1ed4befc7bd19ce885b9eca0b9e99d Mon Sep 17 00:00:00 2001 From: zentol Date: Wed, 21 Feb 2018 17:32:48 +0100 Subject: [PATCH 2/3] remove static empty string array --- .../main/java/org/apache/flink/configuration/CoreOptions.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java index 3e223f9879520..97269c1a271bb 100644 --- a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java +++ b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java @@ -101,14 +101,12 @@ public class CoreOptions { " 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.key() + "\"."); - private static final String[] EMPTY_STRING_ARRAY = new String[0]; - public static String[] getParentFirstLoaderPatterns(Configuration config) { String base = config.getString(ALWAYS_PARENT_FIRST_LOADER); String append = config.getString(ALWAYS_PARENT_FIRST_LOADER_APPEND); String[] basePatterns = base.isEmpty() - ? EMPTY_STRING_ARRAY + ? new String[0] : base.split(";"); if (append.isEmpty()) { From dbc8118b26dccb647aaa4f2a7d25b562da6a5a52 Mon Sep 17 00:00:00 2001 From: zentol Date: Wed, 21 Feb 2018 17:35:00 +0100 Subject: [PATCH 3/3] base -> default, append -> additional --- docs/ops/config.md | 8 ++++---- .../flink/configuration/CoreOptions.java | 18 +++++++++--------- .../flink/configuration/CoreOptionsTest.java | 8 ++++---- .../configuration/ParentFirstPatternsTest.java | 2 +- .../formats/avro/AvroKryoClassloadingTest.java | 2 +- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/docs/ops/config.md b/docs/ops/config.md index f249a275f2cba..ce5bc9b981bee 100644 --- a/docs/ops/config.md +++ b/docs/ops/config.md @@ -75,16 +75,16 @@ without explicit scheme definition, such as `/user/USERNAME/in.txt`, is going to - `classloader.resolve-order`: Whether Flink should use a child-first `ClassLoader` when loading user-code classes or a parent-first `ClassLoader`. Can be one of `parent-first` or `child-first`. (default: `child-first`) -- `classloader.parent-first-patterns.base`: A (semicolon-separated) list of patterns that specifies which +- `classloader.parent-first-patterns.default`: A (semicolon-separated) list of patterns that specifies which classes should always be resolved through the parent `ClassLoader` first. A pattern is a simple prefix that is checked against the fully qualified class name. By default, this is set to `"java.;scala.;org.apache.flink.;com.esotericsoftware.kryo;org.apache.hadoop.;javax.annotation.;org.slf4j;org.apache.log4j;org.apache.logging.log4j;ch.qos.logback"`. -To extend this list beyond the default it is recommended to configure `classloader.parent-first-patterns.append` instead of modifying this setting directly. +To extend this list beyond the default it is recommended to configure `classloader.parent-first-patterns.additional` instead of modifying this setting directly. -- `classloader.parent-first-patterns.append`: A (semicolon-separated) list of patterns that specifies which +- `classloader.parent-first-patterns.additional`: A (semicolon-separated) list of patterns that specifies which classes should always be resolved through the parent `ClassLoader` first. A pattern is a simple prefix that is checked against the fully qualified class name. -This list is appended to `classloader.parent-first-patterns.base`. +This list is appended to `classloader.parent-first-patterns.default`. ## Advanced Options diff --git a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java index 97269c1a271bb..2c02aeb68a2f0 100644 --- a/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java +++ b/flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java @@ -45,7 +45,7 @@ public class CoreOptions { * which means that user code jars can include and load different dependencies than * Flink uses (transitively). * - *

Exceptions to the rules are defined via {@link #ALWAYS_PARENT_FIRST_LOADER}. + *

Exceptions to the rules are defined via {@link #ALWAYS_PARENT_FIRST_LOADER_PATTERNS}. */ public static final ConfigOption CLASSLOADER_RESOLVE_ORDER = ConfigOptions .key("classloader.resolve-order") @@ -85,25 +85,25 @@ public class CoreOptions { * log bindings. * */ - public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER = ConfigOptions - .key("classloader.parent-first-patterns.base") + public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER_PATTERNS = ConfigOptions + .key("classloader.parent-first-patterns.default") .defaultValue("java.;scala.;org.apache.flink.;com.esotericsoftware.kryo;org.apache.hadoop.;javax.annotation.;org.slf4j;org.apache.log4j;org.apache.logging.log4j;ch.qos.logback") .withDeprecatedKeys("classloader.parent-first-patterns") .withDescription("A (semicolon-separated) list of patterns that specifies which classes should always be" + " resolved through the parent ClassLoader first. A pattern is a simple prefix that is checked against" + " the fully qualified class name. This setting should generally not be modified. To add another pattern we" + - " recommend to use \"classloader.parent-first-patterns.append\" instead."); + " recommend to use \"classloader.parent-first-patterns.additional\" instead."); - public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER_APPEND = ConfigOptions - .key("classloader.parent-first-patterns.append") + public static final ConfigOption ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL = ConfigOptions + .key("classloader.parent-first-patterns.additional") .defaultValue("") .withDescription("A (semicolon-separated) list of patterns that specifies which classes should always be" + " 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.key() + "\"."); + " the fully qualified class name. These patterns are appended to \"" + ALWAYS_PARENT_FIRST_LOADER_PATTERNS.key() + "\"."); public static String[] getParentFirstLoaderPatterns(Configuration config) { - String base = config.getString(ALWAYS_PARENT_FIRST_LOADER); - String append = config.getString(ALWAYS_PARENT_FIRST_LOADER_APPEND); + String base = config.getString(ALWAYS_PARENT_FIRST_LOADER_PATTERNS); + String append = config.getString(ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL); String[] basePatterns = base.isEmpty() ? new String[0] diff --git a/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java b/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java index f71fd827f3bdb..0bc59104e5734 100644 --- a/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java +++ b/flink-core/src/test/java/org/apache/flink/configuration/CoreOptionsTest.java @@ -30,22 +30,22 @@ public void testGetParentFirstLoaderPatterns() { Configuration config = new Configuration(); Assert.assertArrayEquals( - CoreOptions.ALWAYS_PARENT_FIRST_LOADER.defaultValue().split(";"), + CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS.defaultValue().split(";"), CoreOptions.getParentFirstLoaderPatterns(config)); - config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER, "hello;world"); + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS, "hello;world"); Assert.assertArrayEquals( "hello;world".split(";"), CoreOptions.getParentFirstLoaderPatterns(config)); - config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_APPEND, "how;are;you"); + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL, "how;are;you"); Assert.assertArrayEquals( "hello;world;how;are;you".split(";"), CoreOptions.getParentFirstLoaderPatterns(config)); - config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER, ""); + config.setString(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS, ""); Assert.assertArrayEquals( "how;are;you".split(";"), diff --git a/flink-core/src/test/java/org/apache/flink/configuration/ParentFirstPatternsTest.java b/flink-core/src/test/java/org/apache/flink/configuration/ParentFirstPatternsTest.java index 784d0998768b4..ca4b511f45636 100644 --- a/flink-core/src/test/java/org/apache/flink/configuration/ParentFirstPatternsTest.java +++ b/flink-core/src/test/java/org/apache/flink/configuration/ParentFirstPatternsTest.java @@ -34,7 +34,7 @@ public class ParentFirstPatternsTest extends TestLogger { private static final HashSet PARENT_FIRST_PACKAGES = new HashSet<>( - Arrays.asList(CoreOptions.ALWAYS_PARENT_FIRST_LOADER.defaultValue().split(";"))); + Arrays.asList(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS.defaultValue().split(";"))); /** * All java and Flink classes must be loaded parent first. diff --git a/flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java b/flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java index 6eaca15240efd..8f0916cfd6698 100644 --- a/flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java +++ b/flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java @@ -73,7 +73,7 @@ public void testKryoInChildClasspath() throws Exception { final ClassLoader userAppClassLoader = FlinkUserCodeClassLoaders.childFirst( new URL[] { avroLocation, kryoLocation }, parentClassLoader, - CoreOptions.ALWAYS_PARENT_FIRST_LOADER.defaultValue().split(";")); + CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS.defaultValue().split(";")); final Class userLoadedAvroClass = Class.forName(avroClass.getName(), false, userAppClassLoader); assertNotEquals(avroClass, userLoadedAvroClass);