flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From uce <...@git.apache.org>
Subject [GitHub] flink pull request #2123: [FLINK-3904] GlobalConfiguration doesn't ensure co...
Date Mon, 25 Jul 2016 14:26:14 GMT
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.
---

Mime
View raw message