This is an automated email from the ASF dual-hosted git repository. iluo pushed a commit to branch dev-metadata in repository https://gitbox.apache.org/repos/asf/incubator-dubbo.git
The following commit(s) were added to refs/heads/dev-metadata by this push: new 3dfa8d2 Code review around Environment, AbstractConfig, and AbstractPrefixConfiguration's impls. (#2820) 3dfa8d2 is described below commit 3dfa8d24e4c0ca6f39c5257c06c8524418a33229 Author: Ian Luo <ian....@gmail.com> AuthorDate: Fri Nov 23 10:20:32 2018 +0800 Code review around Environment, AbstractConfig, and AbstractPrefixConfiguration's impls. (#2820) * add javadoc, comments, and FIXME * move appconfig logic into Environment from refresh method * enhance naming in Environment * introduce calculatePrefix to avoid dup code * roll back calculatePrefix(). --- .../apache/dubbo/common/config/Environment.java | 77 ++++++++++++++-------- .../common/config/EnvironmentConfiguration.java | 2 +- .../dubbo/common/config/InmemoryConfiguration.java | 14 ++-- .../common/config/PropertiesConfiguration.java | 3 + .../dubbo/common/config/SystemConfiguration.java | 5 +- .../org/apache/dubbo/config/AbstractConfig.java | 24 +++++-- .../dubbo/config/spring/ConfigCenterBean.java | 4 +- .../dubbo/configcenter/ConfigurationUtils.java | 8 +-- 8 files changed, 86 insertions(+), 51 deletions(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/config/Environment.java b/dubbo-common/src/main/java/org/apache/dubbo/common/config/Environment.java index 1d94c9f..a6ce939 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/config/Environment.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/config/Environment.java @@ -29,13 +29,14 @@ import java.util.concurrent.ConcurrentHashMap; public class Environment { private static final Environment INSTANCE = new Environment(); - private Map<String, PropertiesConfiguration> propertiesConfsHolder = new ConcurrentHashMap<>(); - private Map<String, SystemConfiguration> systemConfsHolder = new ConcurrentHashMap<>(); - private Map<String, EnvironmentConfiguration> environmentConfsHolder = new ConcurrentHashMap<>(); - private Map<String, InmemoryConfiguration> externalConfsHolder = new ConcurrentHashMap<>(); - private Map<String, InmemoryConfiguration> appExternalConfsHolder = new ConcurrentHashMap<>(); + private Map<String, PropertiesConfiguration> propertiesConfigs = new ConcurrentHashMap<>(); + private Map<String, SystemConfiguration> systemConfigs = new ConcurrentHashMap<>(); + private Map<String, EnvironmentConfiguration> environmentConfigs = new ConcurrentHashMap<>(); + private Map<String, InmemoryConfiguration> externalConfigs = new ConcurrentHashMap<>(); + private Map<String, InmemoryConfiguration> appExternalConfigs = new ConcurrentHashMap<>(); + private Map<String, InmemoryConfiguration> appConfigs = new ConcurrentHashMap<>(); - private boolean isConfigCenterFirst = true; + private boolean configCenterFirst = true; private Map<String, String> externalConfigurationMap = new HashMap<>(); private Map<String, String> appExternalConfigurationMap = new HashMap<>(); @@ -44,42 +45,54 @@ public class Environment { return INSTANCE; } - public PropertiesConfiguration getPropertiesConf(String prefix, String id) { - return propertiesConfsHolder.computeIfAbsent(toKey(prefix, id), k -> new PropertiesConfiguration(prefix, id)); + public PropertiesConfiguration getPropertiesConfig(String prefix, String id) { + return propertiesConfigs.computeIfAbsent(toKey(prefix, id), k -> new PropertiesConfiguration(prefix, id)); } - public SystemConfiguration getSystemConf(String prefix, String id) { - return systemConfsHolder.computeIfAbsent(toKey(prefix, id), k -> new SystemConfiguration(prefix, id)); + public SystemConfiguration getSystemConfig(String prefix, String id) { + return systemConfigs.computeIfAbsent(toKey(prefix, id), k -> new SystemConfiguration(prefix, id)); } - public InmemoryConfiguration getExternalConfiguration(String prefix, String id) { - return externalConfsHolder.computeIfAbsent(toKey(prefix, id), k -> { + public InmemoryConfiguration getExternalConfig(String prefix, String id) { + return externalConfigs.computeIfAbsent(toKey(prefix, id), k -> { InmemoryConfiguration configuration = new InmemoryConfiguration(prefix, id); configuration.addProperties(externalConfigurationMap); return configuration; }); } - public InmemoryConfiguration getAppExternalConfiguration(String prefix, String id) { - return appExternalConfsHolder.computeIfAbsent(toKey(prefix, id), k -> { + public InmemoryConfiguration getAppExternalConfig(String prefix, String id) { + return appExternalConfigs.computeIfAbsent(toKey(prefix, id), k -> { InmemoryConfiguration configuration = new InmemoryConfiguration(prefix, id); configuration.addProperties(appExternalConfigurationMap); return configuration; }); } - public EnvironmentConfiguration getEnvironmentConf(String prefix, String id) { - return environmentConfsHolder.computeIfAbsent(toKey(prefix, id), k -> new EnvironmentConfiguration(prefix, id)); + public EnvironmentConfiguration getEnvironmentConfig(String prefix, String id) { + return environmentConfigs.computeIfAbsent(toKey(prefix, id), k -> new EnvironmentConfiguration(prefix, id)); } - public synchronized void setExternalConfiguration(Map<String, String> externalConfiguration) { + public InmemoryConfiguration getAppConfig(String prefix, String id) { + return appConfigs.get(toKey(prefix, id)); + } + + public synchronized void setExternalConfig(Map<String, String> externalConfiguration) { this.externalConfigurationMap = externalConfiguration; } - public synchronized void setAppExternalConfiguration(Map<String, String> appExternalConfiguration) { + public synchronized void setAppExternalConfig(Map<String, String> appExternalConfiguration) { this.appExternalConfigurationMap = appExternalConfiguration; } + public void addAppConfig(String prefix, String id, Map<String, String> properties) { + appConfigs.computeIfAbsent(toKey(prefix, id), k -> { + InmemoryConfiguration configuration = new InmemoryConfiguration(prefix, id); + configuration.addProperties(properties); + return configuration; + }); + } + public void updateExternalConfigurationMap(Map<String, String> externalMap) { this.externalConfigurationMap.putAll(externalMap); } @@ -99,20 +112,26 @@ public class Environment { */ public CompositeConfiguration getStartupCompositeConf(String prefix, String id) { CompositeConfiguration compositeConfiguration = new CompositeConfiguration(); - compositeConfiguration.addConfiguration(this.getSystemConf(prefix, id)); - compositeConfiguration.addConfiguration(this.getAppExternalConfiguration(prefix, id)); - compositeConfiguration.addConfiguration(this.getExternalConfiguration(prefix, id)); - compositeConfiguration.addConfiguration(this.getPropertiesConf(prefix, id)); + compositeConfiguration.addConfiguration(this.getSystemConfig(prefix, id)); + compositeConfiguration.addConfiguration(this.getAppExternalConfig(prefix, id)); + compositeConfiguration.addConfiguration(this.getExternalConfig(prefix, id)); + compositeConfiguration.addConfiguration(this.getPropertiesConfig(prefix, id)); + + InmemoryConfiguration appConfig = this.getAppConfig(prefix, id); + if (appConfig != null) { + int index = configCenterFirst ? 3 : 1; + compositeConfiguration.addConfiguration(index, appConfig); + } return compositeConfiguration; } - private static String toKey(String keypart1, String keypart2) { + private static String toKey(String prefix, String id) { StringBuilder sb = new StringBuilder(); - if (StringUtils.isNotEmpty(keypart1)) { - sb.append(keypart1); + if (StringUtils.isNotEmpty(prefix)) { + sb.append(prefix); } - if (StringUtils.isNotEmpty(keypart2)) { - sb.append(keypart2); + if (StringUtils.isNotEmpty(id)) { + sb.append(id); } if (sb.length() > 0 && sb.charAt(sb.length() - 1) != '.') { @@ -126,10 +145,10 @@ public class Environment { } public boolean isConfigCenterFirst() { - return isConfigCenterFirst; + return configCenterFirst; } public void setConfigCenterFirst(boolean configCenterFirst) { - isConfigCenterFirst = configCenterFirst; + this.configCenterFirst = configCenterFirst; } } diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/config/EnvironmentConfiguration.java b/dubbo-common/src/main/java/org/apache/dubbo/common/config/EnvironmentConfiguration.java index a38e831..0f6f00a 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/config/EnvironmentConfiguration.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/config/EnvironmentConfiguration.java @@ -17,7 +17,7 @@ package org.apache.dubbo.common.config; /** - * + * Configuration from system environment */ public class EnvironmentConfiguration extends AbstractPrefixConfiguration { diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/config/InmemoryConfiguration.java b/dubbo-common/src/main/java/org/apache/dubbo/common/config/InmemoryConfiguration.java index a54e941..b4a9c48 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/config/InmemoryConfiguration.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/config/InmemoryConfiguration.java @@ -20,13 +20,11 @@ import java.util.LinkedHashMap; import java.util.Map; /** - * + * In-memory configuration */ public class InmemoryConfiguration extends AbstractPrefixConfiguration { - /** - * stores the configuration key-value pairs - */ + // stores the configuration key-value pairs private Map<String, String> store = new LinkedHashMap<>(); public InmemoryConfiguration(String prefix, String id) { @@ -43,15 +41,15 @@ public class InmemoryConfiguration extends AbstractPrefixConfiguration { } /** - * Replace previous value if there is one - * - * @param key - * @param value + * Add one property into the store, the previous value will be replaced if the key exists */ public void addProperty(String key, String value) { store.put(key, value); } + /** + * Add a set of properties into the store + */ public void addProperties(Map<String, String> properties) { store.putAll(properties); } diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/config/PropertiesConfiguration.java b/dubbo-common/src/main/java/org/apache/dubbo/common/config/PropertiesConfiguration.java index 3a11c31..d76c3a1 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/config/PropertiesConfiguration.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/config/PropertiesConfiguration.java @@ -20,6 +20,9 @@ import org.apache.dubbo.common.logger.Logger; import org.apache.dubbo.common.logger.LoggerFactory; import org.apache.dubbo.common.utils.ConfigUtils; +/** + * Configuration from system properties and dubbo.properties + */ public class PropertiesConfiguration extends AbstractPrefixConfiguration { private static final Logger logger = LoggerFactory.getLogger(PropertiesConfiguration.class); diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/config/SystemConfiguration.java b/dubbo-common/src/main/java/org/apache/dubbo/common/config/SystemConfiguration.java index 853fdf2..2a5bb54 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/config/SystemConfiguration.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/config/SystemConfiguration.java @@ -16,8 +16,11 @@ */ package org.apache.dubbo.common.config; + /** - * + * FIXME: is this really necessary? PropertiesConfiguration should have already covered this: + * @see PropertiesConfiguration + * @See ConfigUtils#getProperty(String) */ public class SystemConfiguration extends AbstractPrefixConfiguration { diff --git a/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractConfig.java b/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractConfig.java index 91e226d..1cb3a30 100644 --- a/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractConfig.java +++ b/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractConfig.java @@ -20,7 +20,6 @@ import org.apache.dubbo.common.Constants; import org.apache.dubbo.common.URL; import org.apache.dubbo.common.config.CompositeConfiguration; import org.apache.dubbo.common.config.Environment; -import org.apache.dubbo.common.config.InmemoryConfiguration; import org.apache.dubbo.common.extension.ExtensionLoader; import org.apache.dubbo.common.logger.Logger; import org.apache.dubbo.common.logger.LoggerFactory; @@ -256,6 +255,21 @@ public abstract class AbstractConfig implements Serializable { } } + /** + * We only check boolean value at this moment. + * + * @param type + * @param value + * @return + */ + private static boolean isTypeMatch(Class<?> type, String value) { + if ((type == boolean.class || type == Boolean.class) + && !("true".equals(value) || "false".equals(value))) { + return false; + } + return true; + } + protected static void checkExtension(Class<?> type, String property, String value) { checkName(property, value); if (value != null && value.length() > 0 @@ -482,11 +496,9 @@ public abstract class AbstractConfig implements Serializable { init = true; try { - InmemoryConfiguration configuration = new InmemoryConfiguration(getPrefix(), getId()); - configuration.addProperties(getMetaData()); - CompositeConfiguration compositeConfiguration = Environment.getInstance().getStartupCompositeConf(getPrefix(), getId()); - int index = Environment.getInstance().isConfigCenterFirst() ? 3 : 1; - compositeConfiguration.addConfiguration(index, configuration); + Environment env = Environment.getInstance(); + env.addAppConfig(getPrefix(), getId(), getMetaData()); + CompositeConfiguration compositeConfiguration = env.getStartupCompositeConf(getPrefix(), getId()); // loop methods, get override value and set the new value back to method Method[] methods = getClass().getMethods(); diff --git a/dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ConfigCenterBean.java b/dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ConfigCenterBean.java index 6c8136f..6b5782e 100644 --- a/dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ConfigCenterBean.java +++ b/dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ConfigCenterBean.java @@ -107,8 +107,8 @@ public class ConfigCenterBean extends ConfigCenterConfig implements Initializing if (auto) { Map<String, String> externalProperties = getConfigurations(getConfigfile(), environment); Map<String, String> appExternalProperties = getConfigurations("application." + getConfigfile(), environment); - org.apache.dubbo.common.config.Environment.getInstance().setExternalConfiguration(externalProperties); - org.apache.dubbo.common.config.Environment.getInstance().setAppExternalConfiguration(appExternalProperties); + org.apache.dubbo.common.config.Environment.getInstance().setExternalConfig(externalProperties); + org.apache.dubbo.common.config.Environment.getInstance().setAppExternalConfig(appExternalProperties); this.init(); } } diff --git a/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationUtils.java b/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationUtils.java index 5068c63..a0c659b 100644 --- a/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationUtils.java +++ b/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationUtils.java @@ -36,10 +36,10 @@ public class ConfigurationUtils { static { compositeConfiguration = new CompositeConfiguration(); compositeConfiguration.addConfiguration(getDynamicConfiguration()); - compositeConfiguration.addConfiguration(Environment.getInstance().getAppExternalConfiguration(null, null)); - compositeConfiguration.addConfiguration(Environment.getInstance().getExternalConfiguration(null, null)); - compositeConfiguration.addConfiguration(Environment.getInstance().getSystemConf(null, null)); - compositeConfiguration.addConfiguration(Environment.getInstance().getPropertiesConf(null, null)); + compositeConfiguration.addConfiguration(Environment.getInstance().getAppExternalConfig(null, null)); + compositeConfiguration.addConfiguration(Environment.getInstance().getExternalConfig(null, null)); + compositeConfiguration.addConfiguration(Environment.getInstance().getSystemConfig(null, null)); + compositeConfiguration.addConfiguration(Environment.getInstance().getPropertiesConfig(null, null)); } private volatile Map<String, CompositeConfiguration> runtimeCompositeConfsHolder = new ConcurrentHashMap<>();