[FLINK-23358][core] Refactor CoreOptions parent first patterns to Lis…

…t options This closes apache#16821
niklassemmler · Feb 3, 2022 · c0505d5 · c0505d5
1 parent fcd42a3
commit c0505d5
Show file tree

Hide file tree

Showing 7 changed files with 88 additions and 59 deletions.
diff --git a/docs/layouts/shortcodes/generated/core_configuration.html b/docs/layouts/shortcodes/generated/core_configuration.html
@@ -24,14 +24,14 @@
         </tr>
         <tr>
             <td><h5>classloader.parent-first-patterns.additional</h5></td>
-            <td style="word-wrap: break-word;">(none)</td>
-            <td>String</td>
+            <td style="word-wrap: break-word;"></td>
+            <td>List&lt;String&gt;</td>
             <td>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 "classloader.parent-first-patterns.default".</td>
         </tr>
         <tr>
             <td><h5>classloader.parent-first-patterns.default</h5></td>
-            <td style="word-wrap: break-word;">"java.;<wbr>scala.;<wbr>org.apache.flink.;<wbr>com.esotericsoftware.kryo;<wbr>org.apache.hadoop.;<wbr>javax.annotation.;<wbr>org.xml;<wbr>javax.xml;<wbr>org.apache.xerces;<wbr>org.w3c;<wbr>org.rocksdb.;<wbr>org.slf4j;<wbr>org.apache.log4j;<wbr>org.apache.logging;<wbr>org.apache.commons.logging;<wbr>ch.qos.logback"</td>
-            <td>String</td>
+            <td style="word-wrap: break-word;">"java.";<wbr>"scala.";<wbr>"org.apache.flink.";<wbr>"com.esotericsoftware.kryo";<wbr>"org.apache.hadoop.";<wbr>"javax.annotation.";<wbr>"org.xml";<wbr>"javax.xml";<wbr>"org.apache.xerces";<wbr>"org.w3c";<wbr>"org.rocksdb.";<wbr>"org.slf4j";<wbr>"org.apache.log4j";<wbr>"org.apache.logging";<wbr>"org.apache.commons.logging";<wbr>"ch.qos.logback"</td>
+            <td>List&lt;String&gt;</td>
             <td>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.additional" instead.</td>
         </tr>
         <tr>

diff --git a/docs/layouts/shortcodes/generated/expert_class_loading_section.html b/docs/layouts/shortcodes/generated/expert_class_loading_section.html
@@ -24,14 +24,14 @@
         </tr>
         <tr>
             <td><h5>classloader.parent-first-patterns.additional</h5></td>
-            <td style="word-wrap: break-word;">(none)</td>
-            <td>String</td>
+            <td style="word-wrap: break-word;"></td>
+            <td>List&lt;String&gt;</td>
             <td>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 "classloader.parent-first-patterns.default".</td>
         </tr>
         <tr>
             <td><h5>classloader.parent-first-patterns.default</h5></td>
-            <td style="word-wrap: break-word;">"java.;<wbr>scala.;<wbr>org.apache.flink.;<wbr>com.esotericsoftware.kryo;<wbr>org.apache.hadoop.;<wbr>javax.annotation.;<wbr>org.xml;<wbr>javax.xml;<wbr>org.apache.xerces;<wbr>org.w3c;<wbr>org.rocksdb.;<wbr>org.slf4j;<wbr>org.apache.log4j;<wbr>org.apache.logging;<wbr>org.apache.commons.logging;<wbr>ch.qos.logback"</td>
-            <td>String</td>
+            <td style="word-wrap: break-word;">"java.";<wbr>"scala.";<wbr>"org.apache.flink.";<wbr>"com.esotericsoftware.kryo";<wbr>"org.apache.hadoop.";<wbr>"javax.annotation.";<wbr>"org.xml";<wbr>"javax.xml";<wbr>"org.apache.xerces";<wbr>"org.w3c";<wbr>"org.rocksdb.";<wbr>"org.slf4j";<wbr>"org.apache.log4j";<wbr>"org.apache.logging";<wbr>"org.apache.commons.logging";<wbr>"ch.qos.logback"</td>
+            <td>List&lt;String&gt;</td>
             <td>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.additional" instead.</td>
         </tr>
         <tr>

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
@@ -24,10 +24,12 @@
 import org.apache.flink.annotation.docs.ConfigGroups;
 import org.apache.flink.annotation.docs.Documentation;
 import org.apache.flink.configuration.description.Description;
+import org.apache.flink.util.ArrayUtils;
 
-import org.apache.flink.shaded.guava30.com.google.common.base.Splitter;
 import org.apache.flink.shaded.guava30.com.google.common.collect.Iterables;
 
+import java.util.List;
+
 import static org.apache.flink.configuration.ConfigOptions.key;
 
 /** The set of configuration options for core parameters. */
@@ -36,8 +38,14 @@
 public class CoreOptions {
 
     @Internal
-    public static final String PARENT_FIRST_LOGGING_PATTERNS =
-            "org.slf4j;org.apache.log4j;org.apache.logging;org.apache.commons.logging;ch.qos.logback";
+    public static final String[] PARENT_FIRST_LOGGING_PATTERNS =
+            new String[] {
+                "org.slf4j",
+                "org.apache.log4j",
+                "org.apache.logging",
+                "org.apache.commons.logging",
+                "ch.qos.logback"
+            };
 
     // ------------------------------------------------------------------------
     //  Classloading Parameters
@@ -94,11 +102,26 @@ public class CoreOptions {
      * </ul>
      */
     @Documentation.Section(Documentation.Sections.EXPERT_CLASS_LOADING)
-    public static final ConfigOption<String> ALWAYS_PARENT_FIRST_LOADER_PATTERNS =
+    public static final ConfigOption<List<String>> 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.xml;javax.xml;org.apache.xerces;org.w3c;org.rocksdb.;"
-                                    + PARENT_FIRST_LOGGING_PATTERNS)
+                    .stringType()
+                    .asList()
+                    .defaultValues(
+                            ArrayUtils.concat(
+                                    new String[] {
+                                        "java.",
+                                        "scala.",
+                                        "org.apache.flink.",
+                                        "com.esotericsoftware.kryo",
+                                        "org.apache.hadoop.",
+                                        "javax.annotation.",
+                                        "org.xml",
+                                        "javax.xml",
+                                        "org.apache.xerces",
+                                        "org.w3c",
+                                        "org.rocksdb."
+                                    },
+                                    PARENT_FIRST_LOGGING_PATTERNS))
                     .withDeprecatedKeys("classloader.parent-first-patterns")
                     .withDescription(
                             "A (semicolon-separated) list of patterns that specifies which classes should always be"
@@ -107,9 +130,11 @@ public class CoreOptions {
                                     + " recommend to use \"classloader.parent-first-patterns.additional\" instead.");
 
     @Documentation.Section(Documentation.Sections.EXPERT_CLASS_LOADING)
-    public static final ConfigOption<String> ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL =
+    public static final ConfigOption<List<String>> ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL =
             ConfigOptions.key("classloader.parent-first-patterns.additional")
-                    .defaultValue("")
+                    .stringType()
+                    .asList()
+                    .defaultValues()
                     .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"
@@ -127,9 +152,9 @@ public class CoreOptions {
                                     + "thrown while trying to load a user code class.");
 
     public static String[] getParentFirstLoaderPatterns(Configuration config) {
-        String base = config.getString(ALWAYS_PARENT_FIRST_LOADER_PATTERNS);
-        String append = config.getString(ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL);
-        return parseParentFirstLoaderPatterns(base, append);
+        List<String> base = config.get(ALWAYS_PARENT_FIRST_LOADER_PATTERNS);
+        List<String> append = config.get(ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL);
+        return mergeListsToArray(base, append);
     }
 
     @Documentation.Section(Documentation.Sections.EXPERT_CLASS_LOADING)
@@ -151,12 +176,16 @@ public static String[] getParentFirstLoaderPatterns(Configuration config) {
     @Documentation.ExcludeFromDocumentation(
             "Plugin classloader list is considered an implementation detail. "
                     + "Configuration only included in case to mitigate unintended side-effects of this young feature.")
-    public static final ConfigOption<String> PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS =
+    public static final ConfigOption<List<String>> PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS =
             ConfigOptions.key("plugin.classloader.parent-first-patterns.default")
                     .stringType()
-                    .defaultValue(
-                            "java.;org.apache.flink.;javax.annotation.;"
-                                    + PARENT_FIRST_LOGGING_PATTERNS)
+                    .asList()
+                    .defaultValues(
+                            ArrayUtils.concat(
+                                    new String[] {
+                                        "java.", "org.apache.flink.", "javax.annotation."
+                                    },
+                                    PARENT_FIRST_LOGGING_PATTERNS))
                     .withDescription(
                             "A (semicolon-separated) list of patterns that specifies which classes should always be"
                                     + " resolved through the plugin parent ClassLoader first. A pattern is a simple prefix that is checked "
@@ -166,28 +195,28 @@ public static String[] getParentFirstLoaderPatterns(Configuration config) {
     @Documentation.ExcludeFromDocumentation(
             "Plugin classloader list is considered an implementation detail. "
                     + "Configuration only included in case to mitigate unintended side-effects of this young feature.")
-    public static final ConfigOption<String> PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL =
-            ConfigOptions.key("plugin.classloader.parent-first-patterns.additional")
-                    .stringType()
-                    .defaultValue("")
-                    .withDescription(
-                            "A (semicolon-separated) list of patterns that specifies which classes should always be"
-                                    + " resolved through the plugin parent ClassLoader first. A pattern is a simple prefix that is checked "
-                                    + " against the fully qualified class name. These patterns are appended to \""
-                                    + PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS.key()
-                                    + "\".");
+    public static final ConfigOption<List<String>>
+            PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL =
+                    ConfigOptions.key("plugin.classloader.parent-first-patterns.additional")
+                            .stringType()
+                            .asList()
+                            .defaultValues()
+                            .withDescription(
+                                    "A (semicolon-separated) list of patterns that specifies which classes should always be"
+                                            + " resolved through the plugin parent ClassLoader first. A pattern is a simple prefix that is checked "
+                                            + " against the fully qualified class name. These patterns are appended to \""
+                                            + PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS.key()
+                                            + "\".");
 
     public static String[] getPluginParentFirstLoaderPatterns(Configuration config) {
-        String base = config.getString(PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS);
-        String append = config.getString(PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL);
-        return parseParentFirstLoaderPatterns(base, append);
+        List<String> base = config.get(PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS);
+        List<String> append = config.get(PLUGIN_ALWAYS_PARENT_FIRST_LOADER_PATTERNS_ADDITIONAL);
+        return mergeListsToArray(base, append);
     }
 
     @Internal
-    public static String[] parseParentFirstLoaderPatterns(String base, String append) {
-        Splitter splitter = Splitter.on(';').omitEmptyStrings();
-        return Iterables.toArray(
-                Iterables.concat(splitter.split(base), splitter.split(append)), String.class);
+    public static String[] mergeListsToArray(List<String> base, List<String> append) {
+        return Iterables.toArray(Iterables.concat(base, append), String.class);
     }
 
     // ------------------------------------------------------------------------

diff --git a/flink-core/src/main/java/org/apache/flink/core/classloading/SubmoduleClassLoader.java b/flink-core/src/main/java/org/apache/flink/core/classloading/SubmoduleClassLoader.java
@@ -38,8 +38,7 @@ public SubmoduleClassLoader(URL[] classpath, ClassLoader parentClassLoader) {
         super(
                 classpath,
                 parentClassLoader,
-                CoreOptions.parseParentFirstLoaderPatterns(
-                        CoreOptions.PARENT_FIRST_LOGGING_PATTERNS, ""),
+                CoreOptions.PARENT_FIRST_LOGGING_PATTERNS,
                 new String[] {"org.apache.flink"});
     }
 }
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
@@ -21,6 +21,9 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 import java.util.function.Function;
 
 /** Tests for {@link CoreOptions}. */
@@ -43,22 +46,23 @@ public void testGetPluginParentFirstLoaderPatterns() {
 
     private void testParentFirst(
             Function<Configuration, String[]> patternGetter,
-            ConfigOption<String> patternOption,
-            ConfigOption<String> additionalOption) {
+            ConfigOption<List<String>> patternOption,
+            ConfigOption<List<String>> additionalOption) {
         Configuration config = new Configuration();
         Assert.assertArrayEquals(
-                patternOption.defaultValue().split(";"), patternGetter.apply(config));
+                patternOption.defaultValue().toArray(new String[0]), patternGetter.apply(config));
 
-        config.setString(patternOption, "hello;world");
+        config.set(patternOption, Arrays.asList("hello", "world"));
 
-        Assert.assertArrayEquals("hello;world".split(";"), patternGetter.apply(config));
+        Assert.assertArrayEquals(new String[] {"hello", "world"}, patternGetter.apply(config));
 
-        config.setString(additionalOption, "how;are;you");
+        config.set(additionalOption, Arrays.asList("how", "are", "you"));
 
-        Assert.assertArrayEquals("hello;world;how;are;you".split(";"), patternGetter.apply(config));
+        Assert.assertArrayEquals(
+                new String[] {"hello", "world", "how", "are", "you"}, patternGetter.apply(config));
 
-        config.setString(patternOption, "");
+        config.set(patternOption, Collections.emptyList());
 
-        Assert.assertArrayEquals("how;are;you".split(";"), patternGetter.apply(config));
+        Assert.assertArrayEquals(new String[] {"how", "are", "you"}, patternGetter.apply(config));
     }
 }
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
@@ -22,7 +22,6 @@
 
 import org.junit.Test;
 
-import java.util.Arrays;
 import java.util.HashSet;
 
 import static org.junit.Assert.assertTrue;
@@ -34,11 +33,7 @@
 public class ParentFirstPatternsTest extends TestLogger {
 
     private static final HashSet<String> PARENT_FIRST_PACKAGES =
-            new HashSet<>(
-                    Arrays.asList(
-                            CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS
-                                    .defaultValue()
-                                    .split(";")));
+            new HashSet<>(CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS.defaultValue());
 
     /** All java and Flink classes must be loaded parent first. */
     @Test

diff --git a/...mats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java b/...mats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroKryoClassloadingTest.java
@@ -76,7 +76,9 @@ public void testKryoInChildClasspath() throws Exception {
                 FlinkUserCodeClassLoaders.childFirst(
                         new URL[] {avroLocation, kryoLocation},
                         parentClassLoader,
-                        CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS.defaultValue().split(";"),
+                        CoreOptions.ALWAYS_PARENT_FIRST_LOADER_PATTERNS
+                                .defaultValue()
+                                .toArray(new String[0]),
                         NOOP_EXCEPTION_HANDLER,
                         true);