You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by mxm <gi...@git.apache.org> on 2016/06/17 10:44:13 UTC

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

GitHub user mxm opened a pull request:

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

    [FLINK-3904] GlobalConfiguration doesn't ensure config has been loaded

    This PR contains two commits. The first commit simply checks whether `loadConfiguration(..)` has been called. The second commit adds additional tests and forbids malformed XML files.

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

    $ git pull https://github.com/mxm/flink FLINK-3904

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

    https://github.com/apache/flink/pull/2123.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 #2123
    
----
commit cd5ed8a47db2e17f0adeef14752066feb15aa413
Author: Maximilian Michels <mx...@apache.org>
Date:   2016-06-17T09:20:03Z

    [FLINK-3904] GlobalConfiguration doesn't ensure config has been loaded

commit 3dc3d4d51c7d44be480481236ae45681e6cbe183
Author: Maximilian Michels <mx...@apache.org>
Date:   2016-06-17T09:46:11Z

    harden checks for loaded config
    
    - Doesn't return the GlobalConfiguration if config couldn't be loaded
    - Don't allow malformed xml files
    - Adds tests
    - Reformat tests

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    Brief summary of changes:
    
    - fail if config couldn't be loaded
    - make globalconfiguration non-global and remove static SINGLETON
    - remove duplicate api methods
    - remove undocumented XML loading feature
    - generate yaml conf in tests instead of xml conf
    - only load one config file instead of all xml or yaml files (flink-conf.yaml)
    - fix test cases
    - add test cases


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    This problem sounds like it might cause very hard to debug problems.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72076345
  
    --- Diff: flink-core/src/main/java/org/apache/flink/api/common/io/DelimitedInputFormat.java ---
    @@ -84,12 +84,11 @@
     	 */
     	private static int MAX_SAMPLE_LEN;
     
    -	static { loadGlobalConfigParams(); }
    -	
    -	protected static void loadGlobalConfigParams() {
    -		int maxSamples = GlobalConfiguration.getInteger(ConfigConstants.DELIMITED_FORMAT_MAX_LINE_SAMPLES_KEY,
    +
    +	protected static void loadConfigParameters(Configuration parameters) {
    --- End diff --
    
    According to the plugin which checks API changes, it is not breaking. I don't know whether this is a shortcoming of the plugin or actually ok. We can still keep the old method.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    That's odd. I was working on exactly these changes and have just pushed them (without seeing your comment before).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72076940
  
    --- Diff: flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java ---
    @@ -44,159 +35,62 @@
     @Internal
     public final class GlobalConfiguration {
     
    -	/** The log object used for debugging. */
     	private static final Logger LOG = LoggerFactory.getLogger(GlobalConfiguration.class);
     
    -	/** The global configuration object accessible through a singleton pattern. */
    -	private static GlobalConfiguration SINGLETON = null;
    -
    -	/** The internal map holding the key-value pairs the configuration consists of. */
    -	private final Configuration config = new Configuration();
    +	public static final String FLINK_CONF_FILENAME = "flink-conf.yaml";
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Retrieves the singleton object of the global configuration.
    -	 * 
    -	 * @return the global configuration object
    -	 */
    -	private static GlobalConfiguration get() {
    -		// lazy initialization currently only for testibility
    -		synchronized (GlobalConfiguration.class) {
    -			if (SINGLETON == null) {
    -				SINGLETON = new GlobalConfiguration();
    -			}
    -			return SINGLETON;
    -		}
    -	}
     
    -	/**
    -	 * The constructor used to construct the singleton instance of the global configuration.
    -	 */
     	private GlobalConfiguration() {}
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Returns the value associated with the given key as a string.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static String getString(String key, String defaultValue) {
    -		return get().config.getString(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a long integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static long getLong(String key, long defaultValue) {
    -		return get().config.getLong(key, defaultValue);
    -	}
     
     	/**
    -	 * Returns the value associated with the given key as an integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static int getInteger(String key, int defaultValue) {
    -		return get().config.getInteger(key, defaultValue);
    -	}
    -	
    -	/**
    -	 * Returns the value associated with the given key as a float.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    +	 * Loads the global configuration from the environment. Fails if an error occurs during loading. Returns an
    +	 * empty configuration object if the environment variable is not set. In production this variable is set but
    +	 * tests and local execution/debugging don't have this environment variable set. That's why we should fail
    +	 * if it is not set.
    +	 * @return Returns the Configuration
     	 */
    -	public static float getFloat(String key, float defaultValue) {
    -		return get().config.getFloat(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a boolean.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static boolean getBoolean(String key, boolean defaultValue) {
    -		return get().config.getBoolean(key, defaultValue);
    +	public static Configuration loadConfiguration() {
    +		final String configDir = System.getenv(ConfigConstants.ENV_FLINK_CONF_DIR);
    +		if (configDir == null) {
    +			return new Configuration();
    +		}
    +		return loadConfiguration(configDir);
     	}
     
     	/**
     	 * Loads the configuration files from the specified directory.
     	 * <p>
    -	 * XML and YAML are supported as configuration files. If both XML and YAML files exist in the configuration
    -	 * directory, keys from YAML will overwrite keys from XML.
    +	 * YAML files are supported as configuration files.
     	 * 
     	 * @param configDir
     	 *        the directory which contains the configuration files
     	 */
    -	public static void loadConfiguration(final String configDir) {
    +	public static Configuration loadConfiguration(final String configDir) {
     
     		if (configDir == null) {
    -			LOG.warn("Given configuration directory is null, cannot load configuration");
    -			return;
    +			throw new IllegalConfigurationException("Given configuration directory is null, cannot load configuration");
    --- End diff --
    
    Do you mean replacing this with 
    ```java
    Preconditions.checkArgument(configDir != null, "Given configuration directory is null, cannot load configuration")`
    ```
    ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72074912
  
    --- Diff: flink-core/src/test/resources/log4j-test.properties ---
    @@ -18,7 +18,7 @@
     
     # Set root logger level to OFF to not flood build logs
     # set manually to INFO for debugging purposes
    -log4j.rootLogger=OFF, testlogger
    +log4j.rootLogger=ERROR, testlogger
    --- End diff --
    
    We can. I actually found this useful for debugging because you generally want to see ERRORs :) Can be resolved in a different issue though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72074124
  
    --- Diff: flink-core/src/test/resources/log4j-test.properties ---
    @@ -18,7 +18,7 @@
     
     # Set root logger level to OFF to not flood build logs
     # set manually to INFO for debugging purposes
    -log4j.rootLogger=OFF, testlogger
    +log4j.rootLogger=ERROR, testlogger
    --- End diff --
    
    Should we set this back to `OFF` before merging?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    Rebased to latest master. Merging this once Travis passes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72078054
  
    --- Diff: flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java ---
    @@ -44,159 +35,62 @@
     @Internal
     public final class GlobalConfiguration {
     
    -	/** The log object used for debugging. */
     	private static final Logger LOG = LoggerFactory.getLogger(GlobalConfiguration.class);
     
    -	/** The global configuration object accessible through a singleton pattern. */
    -	private static GlobalConfiguration SINGLETON = null;
    -
    -	/** The internal map holding the key-value pairs the configuration consists of. */
    -	private final Configuration config = new Configuration();
    +	public static final String FLINK_CONF_FILENAME = "flink-conf.yaml";
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Retrieves the singleton object of the global configuration.
    -	 * 
    -	 * @return the global configuration object
    -	 */
    -	private static GlobalConfiguration get() {
    -		// lazy initialization currently only for testibility
    -		synchronized (GlobalConfiguration.class) {
    -			if (SINGLETON == null) {
    -				SINGLETON = new GlobalConfiguration();
    -			}
    -			return SINGLETON;
    -		}
    -	}
     
    -	/**
    -	 * The constructor used to construct the singleton instance of the global configuration.
    -	 */
     	private GlobalConfiguration() {}
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Returns the value associated with the given key as a string.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static String getString(String key, String defaultValue) {
    -		return get().config.getString(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a long integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static long getLong(String key, long defaultValue) {
    -		return get().config.getLong(key, defaultValue);
    -	}
     
     	/**
    -	 * Returns the value associated with the given key as an integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static int getInteger(String key, int defaultValue) {
    -		return get().config.getInteger(key, defaultValue);
    -	}
    -	
    -	/**
    -	 * Returns the value associated with the given key as a float.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    +	 * Loads the global configuration from the environment. Fails if an error occurs during loading. Returns an
    +	 * empty configuration object if the environment variable is not set. In production this variable is set but
    +	 * tests and local execution/debugging don't have this environment variable set. That's why we should fail
    +	 * if it is not set.
    +	 * @return Returns the Configuration
     	 */
    -	public static float getFloat(String key, float defaultValue) {
    -		return get().config.getFloat(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a boolean.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static boolean getBoolean(String key, boolean defaultValue) {
    -		return get().config.getBoolean(key, defaultValue);
    +	public static Configuration loadConfiguration() {
    +		final String configDir = System.getenv(ConfigConstants.ENV_FLINK_CONF_DIR);
    +		if (configDir == null) {
    +			return new Configuration();
    +		}
    +		return loadConfiguration(configDir);
     	}
     
     	/**
     	 * Loads the configuration files from the specified directory.
     	 * <p>
    -	 * XML and YAML are supported as configuration files. If both XML and YAML files exist in the configuration
    -	 * directory, keys from YAML will overwrite keys from XML.
    +	 * YAML files are supported as configuration files.
     	 * 
     	 * @param configDir
     	 *        the directory which contains the configuration files
     	 */
    -	public static void loadConfiguration(final String configDir) {
    +	public static Configuration loadConfiguration(final String configDir) {
     
     		if (configDir == null) {
    -			LOG.warn("Given configuration directory is null, cannot load configuration");
    -			return;
    +			throw new IllegalConfigurationException("Given configuration directory is null, cannot load configuration");
    --- End diff --
    
    +1 let's also change the other one


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    Thanks for the review! Although the changes in `DelimitedInputFormat` don't break the API according to the API Maven plugin, we can simply deprecate the existing method and call the new method from it. That should be safe.
    
    Regarding the other changes in behavior (removing XML loading, only loading from one flink-conf.yaml file instead of all Yaml/Xml files found): All these changes are in line with our documentation which doesn't mention the removed features. However, I agree that some users may rely on this. All in all, it might be safer to merge this immediately to the master after we have forked off the release branch.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72072774
  
    --- Diff: flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java ---
    @@ -44,159 +35,62 @@
     @Internal
     public final class GlobalConfiguration {
     
    -	/** The log object used for debugging. */
     	private static final Logger LOG = LoggerFactory.getLogger(GlobalConfiguration.class);
     
    -	/** The global configuration object accessible through a singleton pattern. */
    -	private static GlobalConfiguration SINGLETON = null;
    -
    -	/** The internal map holding the key-value pairs the configuration consists of. */
    -	private final Configuration config = new Configuration();
    +	public static final String FLINK_CONF_FILENAME = "flink-conf.yaml";
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Retrieves the singleton object of the global configuration.
    -	 * 
    -	 * @return the global configuration object
    -	 */
    -	private static GlobalConfiguration get() {
    -		// lazy initialization currently only for testibility
    -		synchronized (GlobalConfiguration.class) {
    -			if (SINGLETON == null) {
    -				SINGLETON = new GlobalConfiguration();
    -			}
    -			return SINGLETON;
    -		}
    -	}
     
    -	/**
    -	 * The constructor used to construct the singleton instance of the global configuration.
    -	 */
     	private GlobalConfiguration() {}
     
     	// --------------------------------------------------------------------------------------------
    -	
    -	/**
    -	 * Returns the value associated with the given key as a string.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static String getString(String key, String defaultValue) {
    -		return get().config.getString(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a long integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static long getLong(String key, long defaultValue) {
    -		return get().config.getLong(key, defaultValue);
    -	}
     
     	/**
    -	 * Returns the value associated with the given key as an integer.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static int getInteger(String key, int defaultValue) {
    -		return get().config.getInteger(key, defaultValue);
    -	}
    -	
    -	/**
    -	 * Returns the value associated with the given key as a float.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    +	 * Loads the global configuration from the environment. Fails if an error occurs during loading. Returns an
    +	 * empty configuration object if the environment variable is not set. In production this variable is set but
    +	 * tests and local execution/debugging don't have this environment variable set. That's why we should fail
    +	 * if it is not set.
    +	 * @return Returns the Configuration
     	 */
    -	public static float getFloat(String key, float defaultValue) {
    -		return get().config.getFloat(key, defaultValue);
    -	}
    -
    -	/**
    -	 * Returns the value associated with the given key as a boolean.
    -	 * 
    -	 * @param key
    -	 *        the key pointing to the associated value
    -	 * @param defaultValue
    -	 *        the default value which is returned in case there is no value associated with the given key
    -	 * @return the (default) value associated with the given key
    -	 */
    -	public static boolean getBoolean(String key, boolean defaultValue) {
    -		return get().config.getBoolean(key, defaultValue);
    +	public static Configuration loadConfiguration() {
    +		final String configDir = System.getenv(ConfigConstants.ENV_FLINK_CONF_DIR);
    +		if (configDir == null) {
    +			return new Configuration();
    +		}
    +		return loadConfiguration(configDir);
     	}
     
     	/**
     	 * Loads the configuration files from the specified directory.
     	 * <p>
    -	 * XML and YAML are supported as configuration files. If both XML and YAML files exist in the configuration
    -	 * directory, keys from YAML will overwrite keys from XML.
    +	 * YAML files are supported as configuration files.
     	 * 
     	 * @param configDir
     	 *        the directory which contains the configuration files
     	 */
    -	public static void loadConfiguration(final String configDir) {
    +	public static Configuration loadConfiguration(final String configDir) {
     
     		if (configDir == null) {
    -			LOG.warn("Given configuration directory is null, cannot load configuration");
    -			return;
    +			throw new IllegalConfigurationException("Given configuration directory is null, cannot load configuration");
    --- End diff --
    
    Should we make this an `IllegalArgumentException` and call the regular `Preconditions.checkNotNull(String, Object)`? If yes, maybe the other `IllegalConfigurationException`, too?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    Very good work Max! The changes look good to me and I think it's much cleaner this way. The `DelimitedInputFormat` API change question needs to be addressed before merging I think.
    
    Regarding the other "API breaking" changes I'm undecided right now. I fully agree that they are not documented and hence should be fine to remove. On the other hand, the code is not expensive to maintain and if someone is relying on loading of multiple YAML files for example, that will be hard to debug for the user. I feel like it might make sense to get more community feedback before merging this for 1.1 release. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2123#discussion_r72071650
  
    --- Diff: flink-core/src/main/java/org/apache/flink/api/common/io/DelimitedInputFormat.java ---
    @@ -84,12 +84,11 @@
     	 */
     	private static int MAX_SAMPLE_LEN;
     
    -	static { loadGlobalConfigParams(); }
    -	
    -	protected static void loadGlobalConfigParams() {
    -		int maxSamples = GlobalConfiguration.getInteger(ConfigConstants.DELIMITED_FORMAT_MAX_LINE_SAMPLES_KEY,
    +
    +	protected static void loadConfigParameters(Configuration parameters) {
    --- End diff --
    
    I think this is a public API change. The class is annotated with `@Public` and the method is `protected`, which means that some users might have extended this input format and rely on this method. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2123: [FLINK-3904] GlobalConfiguration doesn't ensure config ha...

Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/2123
  
    Looks like a good fix for now.
    
    I would eventually really like to get rid of the `GlobalConfiguration` singleton - it causes issues with embedding, testing, and encourages to not cleanly think through designs.
    
    In the end, the `GlobalConfiguration` would only be an XML / YAML loader that returns a `Configuration` object.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---