[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-26 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16377470#comment-16377470
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/5544


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
> Fix For: 1.5.0
>
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-26 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16376715#comment-16376715
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user zentol commented on the issue:

https://github.com/apache/flink/pull/5544
  
merging.


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371807#comment-16371807
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user StephanEwen commented on the issue:

https://github.com/apache/flink/pull/5544
  
Looks good to me.

Would be nice to have another opinion on the config key names. After all, I 
just suggested my personal opinion...


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371637#comment-16371637
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5544#discussion_r169699380
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java ---
@@ -86,11 +86,42 @@
 * 
 */
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];
--- End diff --

I've removed the static field.


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371638#comment-16371638
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user zentol commented on the issue:

https://github.com/apache/flink/pull/5544
  
I've changed the keys to "default" and "additional" and also renamed the 
`ConfigOptions` to `ALWAYS_PARENT_FIRST_LOADER_PATTERNS[_ADDITIONAL]`.


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371505#comment-16371505
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user StephanEwen commented on a diff in the pull request:

https://github.com/apache/flink/pull/5544#discussion_r169660742
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java ---
@@ -86,11 +86,42 @@
 * 
 */
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];
--- End diff --

This is meant well (as in fewer objects), but given that this is used only 
once (or few times), the JVM has more overhead through this additional static 
field (managing class and reflection metadata) than if the method 
`getParentFirstLoaderPatterns()` just does a `new String[0]`, especially 
because it will be a very short lived object.




> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371503#comment-16371503
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

Github user StephanEwen commented on the issue:

https://github.com/apache/flink/pull/5544
  
Good change in principle, but I think "base" and "append" are not a great 
choice of names for the parameters. Probably not very intuitive for users.

How about calling them `default` and `additional`?

While at it, should we also rename the `ALWAYS_PARENT_FIRST_LOADER` config 
key name? Calling it `CLASS_LOADING_PARENT_FIRST_PACKAGES` or 
`CL_PARENT_FIRST_PATTERNS` seems better.


> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-21 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16371441#comment-16371441
 ] 

ASF GitHub Bot commented on FLINK-8645:
---

GitHub user zentol opened a pull request:

https://github.com/apache/flink/pull/5544

[FLINK-8645][configuration] Split classloader.parent-first-patterns into 
"base" and "append

## What is the purpose of the change

This PR splits the  `classloader.parent-first-patterns` option into a 
`base` and `append` to allow users to specify additional patterns without 
having to include the default as well.

## Brief change log

* change key of `ALWAYS_PARENT_FIRST_LOADER` option by appending `.base`
  * add deprecated key `classloader.parent-first-patterns` to not break 
existing configs
* add `ALWAYS_PARENT_FIRST_LOADER_APPEND` option
* add utility method `getParentFirstLoaderPatterns` to `CoreOptions` for 
parsing both the base and append at once from a given configuration
* migrate all production accesses of `ALWAYS_PARENT_FIRST_LOADER` to the 
utility method
* add a test for `getParentFirstLoaderPatterns`
* update documentation
  * update default pattern
  * explicitly recommend the append option for extending the pattern list

## Verifying this change

This change added tests and can be verified as follows:

* run `CoreOptionsTest`

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (yes)
  - The serializers: (ni)
  - The runtime per-record code paths (performance sensitive): (ni)
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  - The S3 file system connector: (no)

## Documentation

  - Does this pull request introduce a new feature? (yes)
  - If yes, how is the feature documented? (docs)


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/zentol/flink 8645

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5544.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #5544


commit 6bf6c6478f7cfbf392eb2cc965651936af2fc606
Author: zentol 
Date:   2018-02-21T13:55:22Z

[FLINK-8645][configuration] Split classloader.parent-first-patterns into 
"base" and "append




> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Assignee: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-8645) Support convenient extension of parent-first ClassLoader patterns

2018-02-13 Thread Aljoscha Krettek (JIRA) 

[ 
https://issues.apache.org/jira/browse/FLINK-8645?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16362630#comment-16362630
 ] 

Aljoscha Krettek commented on FLINK-8645:
-

+1 That makes a lot of sense.

> Support convenient extension of parent-first ClassLoader patterns
> -
>
> Key: FLINK-8645
> URL: https://issues.apache.org/jira/browse/FLINK-8645
> Project: Flink
>  Issue Type: Improvement
>  Components: Configuration
>Affects Versions: 1.5.0
>Reporter: Chesnay Schepler
>Priority: Major
>
> The option {{classloader.parent-first-patterns}} defines a list of class 
> pattern that should always be loaded through the parent class-loader. The 
> default value contains all classes that are effectively required to be loaded 
> that way for Flink to function.
> This list cannot be extended in a convenient way, as one would have to 
> manually copy the existing default and append new entries. This makes the 
> configuration brittle in light of version upgrades where we may extend the 
> default, and also obfuscates the configuration a bit.
> I propose to separate this option into 
> {{classloader.parent-first-patterns.base}}, which subsumes the existing 
> option, and {{classloader.parent-first-patterns.append}} which is 
> automatically appended to the base.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)