This is an automated email from the ASF dual-hosted git repository. liujun 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 607af9d Merge pull request #2817, code review for dubbo-configcente. 607af9d is described below commit 607af9d0d0e0db8ec40bb6a8608333d37946b5b0 Author: Ian Luo <ian....@gmail.com> AuthorDate: Thu Nov 22 12:39:39 2018 +0800 Merge pull request #2817, code review for dubbo-configcente. --- .../dubbo/config/AbstractInterfaceConfig.java | 15 +++-- .../dubbo/configcenter/ConfigurationListener.java | 5 -- .../support/apollo/ApolloDynamicConfiguration.java | 73 +++++++++------------- .../archaius/ArchaiusDynamicConfiguration.java | 40 ++++++------ .../sources/ZooKeeperConfigurationSource.java | 36 ++++++----- .../registry/integration/RegistryProtocol.java | 5 -- 6 files changed, 83 insertions(+), 91 deletions(-) diff --git a/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java b/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java index b1f7223..3840e0c 100644 --- a/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java +++ b/dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java @@ -20,7 +20,6 @@ import org.apache.dubbo.common.Constants; import org.apache.dubbo.common.URL; import org.apache.dubbo.common.Version; import org.apache.dubbo.common.config.Environment; -import org.apache.dubbo.common.extension.ExtensionLoader; import org.apache.dubbo.common.utils.CollectionUtils; import org.apache.dubbo.common.utils.ConfigUtils; import org.apache.dubbo.common.utils.NetUtils; @@ -46,6 +45,7 @@ import java.util.Map; import java.util.Set; import static org.apache.dubbo.common.Constants.APPLICATION_KEY; +import static org.apache.dubbo.common.extension.ExtensionLoader.getExtensionLoader; /** * AbstractDefaultConfig @@ -154,10 +154,17 @@ public abstract class AbstractInterfaceConfig extends AbstractMethodConfig { } } - // For compatibility purpose, use registry as the default config center if the registry protocol is zookeeper and there's no config center specified explicitly. + useRegistryForConfigIfNecessary(); + } + + /** + * For compatibility purpose, use registry as the default config center if the registry protocol is zookeeper and + * there's no config center specified explicitly. + */ + private void useRegistryForConfigIfNecessary() { RegistryConfig registry = registries.get(0); if (registry.isZookeeperProtocol()) { - Set<DynamicConfiguration> loadedConfigurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getExtensions(); + Set<DynamicConfiguration> loadedConfigurations = getExtensionLoader(DynamicConfiguration.class).getExtensions(); // we use the loading status of DynamicConfiguration to decide whether ConfigCenter has been initiated. if (CollectionUtils.isEmpty(loadedConfigurations)) { ConfigCenterConfig configCenterConfig = new ConfigCenterConfig(); @@ -298,7 +305,7 @@ public abstract class AbstractInterfaceConfig extends AbstractMethodConfig { } if (ConfigUtils.isNotEmpty(address)) { if (!map.containsKey(Constants.PROTOCOL_KEY)) { - if (ExtensionLoader.getExtensionLoader(MonitorFactory.class).hasExtension("logstat")) { + if (getExtensionLoader(MonitorFactory.class).hasExtension("logstat")) { map.put(Constants.PROTOCOL_KEY, "logstat"); } else { map.put(Constants.PROTOCOL_KEY, "dubbo"); diff --git a/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java b/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java index bacaa14..7c7a131 100644 --- a/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java +++ b/dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java @@ -16,8 +16,6 @@ */ package org.apache.dubbo.configcenter; -import org.apache.dubbo.common.URL; - /** * Config listener, will get notified when the config it listens on changes. */ @@ -30,7 +28,4 @@ public interface ConfigurationListener { * @param event config change event */ void process(ConfigChangeEvent event); - - // FIXME: why we need this? - URL getUrl(); } diff --git a/dubbo-configcenter/dubbo-configcenter-apollo/src/main/java/org/apache/dubbo/configcenter/support/apollo/ApolloDynamicConfiguration.java b/dubbo-configcenter/dubbo-configcenter-apollo/src/main/java/org/apache/dubbo/configcenter/support/apollo/ApolloDynamicConfiguration.java index efec2fb..8c056dc 100644 --- a/dubbo-configcenter/dubbo-configcenter-apollo/src/main/java/org/apache/dubbo/configcenter/support/apollo/ApolloDynamicConfiguration.java +++ b/dubbo-configcenter/dubbo-configcenter-apollo/src/main/java/org/apache/dubbo/configcenter/support/apollo/ApolloDynamicConfiguration.java @@ -22,22 +22,24 @@ import com.ctrip.framework.apollo.ConfigService; import com.ctrip.framework.apollo.enums.ConfigSourceType; import com.ctrip.framework.apollo.enums.PropertyChangeType; import com.ctrip.framework.apollo.model.ConfigChange; -import com.ctrip.framework.apollo.model.ConfigChangeEvent; + import org.apache.dubbo.common.Constants; import org.apache.dubbo.common.URL; import org.apache.dubbo.common.logger.Logger; import org.apache.dubbo.common.logger.LoggerFactory; import org.apache.dubbo.common.utils.StringUtils; import org.apache.dubbo.configcenter.AbstractDynamicConfiguration; +import org.apache.dubbo.configcenter.ConfigChangeEvent; import org.apache.dubbo.configcenter.ConfigChangeType; -import org.apache.dubbo.configcenter.ConfigType; import org.apache.dubbo.configcenter.ConfigurationListener; -import java.util.HashSet; -import java.util.Set; +import java.util.Collections; + +import static org.apache.dubbo.configcenter.ConfigType.CONFIGURATORS; +import static org.apache.dubbo.configcenter.ConfigType.ROUTERS; /** - * + * Apollo implementation, https://github.com/ctripcorp/apollo */ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<ConfigChangeListener> { private static final Logger logger = LoggerFactory.getLogger(ApolloDynamicConfiguration.class); @@ -54,9 +56,8 @@ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<Con @Override public void initWith(URL url) { super.initWith(url); - /** - * Instead of using Dubbo's configuration, I would suggest use the original configuration method Apollo provides. - */ + + // Instead of using Dubbo's configuration, I would suggest use the original configuration method Apollo provides. String configEnv = url.getParameter(Constants.CONFIG_ENV_KEY); String configAddr = url.getBackupAddress(); String configCluster = url.getParameter(Constants.CONFIG_CLUSTER_KEY); @@ -75,24 +76,20 @@ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<Con boolean check = url.getParameter(Constants.CONFIG_CHECK_KEY, false); if (dubboConfig.getSourceType() != ConfigSourceType.REMOTE) { if (check) { - throw new IllegalStateException("Failed to connect to ConfigCenter, the ConfigCenter is Apollo, the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv)); + throw new IllegalStateException("Failed to connect to config center, the config center is Apollo, " + + "the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv)); } else { - logger.warn("Failed to connect to ConfigCenter, the ConfigCenter is Apollo, " + + logger.warn("Failed to connect to config center, the config center is Apollo, " + "the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv) + - ". will use the local cache value instead before finally connected."); + ", will use the local cache value instead before eventually the connection is established."); } } } /** - * This method will being used to, + * This method will be used to: * 1. get configuration file at startup phase * 2. get all kinds of Dubbo rules - * - * @param key - * @param group - * @param timeout - * @return */ @Override protected String getTargetConfig(String key, String group, long timeout) { @@ -110,9 +107,6 @@ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<Con * This method will used by Configuration to get valid value at runtime. * The group is expected to be 'app level', which can be fetched from the 'config.appnamespace' in url if necessary. * But I think Apollo's inheritance feature of namespace can solve the problem . - * - * @param key - * @return */ @Override protected String getInternalProperty(String key) { @@ -121,9 +115,7 @@ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<Con @Override protected void addTargetListener(String key, ConfigChangeListener listener) { - Set<String> keys = new HashSet<>(1); - keys.add(key); - this.dubboConfig.addChangeListener(listener, keys); + this.dubboConfig.addChangeListener(listener, Collections.singleton(key)); } @Override @@ -131,42 +123,39 @@ public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<Con return new ApolloListener(listener); } - public ConfigChangeType getChangeType(ConfigChange change) { - if (change.getChangeType() == PropertyChangeType.DELETED) { - return ConfigChangeType.DELETED; - } - return ConfigChangeType.MODIFIED; - } - private class ApolloListener implements ConfigChangeListener { - private ConfigurationListener listener; - private URL url; - public ApolloListener(ConfigurationListener listener) { - this(listener.getUrl(), listener); - } + private ConfigurationListener listener; - public ApolloListener(URL url, ConfigurationListener listener) { - this.url = url; + ApolloListener(ConfigurationListener listener) { this.listener = listener; } @Override - public void onChange(ConfigChangeEvent changeEvent) { + public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { for (String key : changeEvent.changedKeys()) { ConfigChange change = changeEvent.getChange(key); if (StringUtils.isEmpty(change.getNewValue())) { - logger.warn("We received an empty rule for " + key + ", the current working rule is " + change.getOldValue() + ", the empty rule will not take effect."); + logger.warn("an empty rule is received for " + key + ", the current working rule is " + + change.getOldValue() + ", the empty rule will not take effect."); return; } - // TODO Maybe we no longer need to identify the type of change. Because there's no scenario that a callback will subscribe for both configurators and routers + // TODO Maybe we no longer need to identify the type of change. Because there's no scenario that + // a callback will subscribe for both configurators and routers if (change.getPropertyName().endsWith(Constants.CONFIGURATORS_SUFFIX)) { - listener.process(new org.apache.dubbo.configcenter.ConfigChangeEvent(key, change.getNewValue(), ConfigType.CONFIGURATORS, getChangeType(change))); + listener.process(new ConfigChangeEvent(key, change.getNewValue(), CONFIGURATORS, getChangeType(change))); } else { - listener.process(new org.apache.dubbo.configcenter.ConfigChangeEvent(key, change.getNewValue(), ConfigType.ROUTERS, getChangeType(change))); + listener.process(new ConfigChangeEvent(key, change.getNewValue(), ROUTERS, getChangeType(change))); } } } + + private ConfigChangeType getChangeType(ConfigChange change) { + if (change.getChangeType() == PropertyChangeType.DELETED) { + return ConfigChangeType.DELETED; + } + return ConfigChangeType.MODIFIED; + } } } diff --git a/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/ArchaiusDynamicConfiguration.java b/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/ArchaiusDynamicConfiguration.java index 9ca06ec..d739009 100644 --- a/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/ArchaiusDynamicConfiguration.java +++ b/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/ArchaiusDynamicConfiguration.java @@ -32,8 +32,16 @@ import org.apache.dubbo.configcenter.ConfigType; import org.apache.dubbo.configcenter.ConfigurationListener; import org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource; +import static org.apache.dubbo.common.Constants.CONFIG_NAMESPACE_KEY; +import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_CHECK_KEY; +import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_ROOT_PATH_KEY; +import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_SOURCE_ADDRESS_KEY; +import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.DEFAULT_CONFIG_ROOT_PATH; + /** * Archaius supports various sources and it's extensiable: JDBC, ZK, Properties, ..., so should we make it extensiable? + * FIXME: we should get rid of Archaius or move it to eco system since Archaius is out of maintenance, instead, we + * should rely on curator-x-async which looks quite promising. */ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<Runnable> { private static final Logger logger = LoggerFactory.getLogger(ArchaiusDynamicConfiguration.class); @@ -50,10 +58,10 @@ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<R String address = url.getBackupAddress(); if (!address.equals(Constants.ANYHOST_VALUE)) { - System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_SOURCE_ADDRESS_KEY, address); + System.setProperty(ARCHAIUS_SOURCE_ADDRESS_KEY, address); } - System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_ROOT_PATH_KEY, url.getParameter(Constants.CONFIG_NAMESPACE_KEY, ZooKeeperConfigurationSource.DEFAULT_CONFIG_ROOT_PATH)); - System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_CHECK_KEY, url.getParameter(Constants.CONFIG_CHECK_KEY, "false")); + System.setProperty(ARCHAIUS_CONFIG_ROOT_PATH_KEY, url.getParameter(CONFIG_NAMESPACE_KEY, DEFAULT_CONFIG_ROOT_PATH)); + System.setProperty(ARCHAIUS_CONFIG_CHECK_KEY, url.getParameter(Constants.CONFIG_CHECK_KEY, "false")); try { ZooKeeperConfigurationSource zkConfigSource = new ZooKeeperConfigurationSource(url); @@ -71,15 +79,11 @@ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<R /** * The hierarchy of configuration properties is: * 1. /{namespace}/config/dubbo/dubbo.properties - * 2. /{namespace}/config/applicationname/dubbo.properties + * 2. /{namespace}/config/{applicationname}/dubbo.properties * <p> - * To make the API compatible with other configuration systems, the key doesn't has group as prefix, so we should add the group prefix before try to get value. - * If being used for dubbo router rules, the key must already contains group prefix. - * - * @param key - * @param group - * @param timeout - * @return + * To make the API compatible with other configuration systems, the key doesn't has group as prefix, so we should + * add the group prefix before try to get value. If being used for dubbo router rules, the key must already + * contains group prefix. */ @Override protected String getTargetConfig(String key, String group, long timeout) { @@ -93,11 +97,7 @@ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<R } /** - * First, get app level configuration - * If there's no value in app level, try to get global dubbo level. - * - * @param key - * @return + * First, get app level configuration. If there's no value in app level, try to get global dubbo level. */ @Override protected Object getInternalProperty(String key) { @@ -120,15 +120,14 @@ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<R private class ArchaiusListener implements Runnable { private ConfigurationListener listener; - private URL url; private String key; private ConfigType type; public ArchaiusListener(String key, ConfigurationListener listener) { this.key = key; this.listener = listener; - this.url = listener.getUrl(); - // Maybe we no longer need to identify the type of change. Because there's no scenario that a callback will subscribe for both configurators and routers + // Maybe we no longer need to identify the type of change. Because there's no scenario that a callback + // will subscribe for both configurators and routers if (key.endsWith(Constants.CONFIGURATORS_SUFFIX)) { type = ConfigType.CONFIGURATORS; } else { @@ -152,7 +151,8 @@ public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<R listener.process(event); } else { if (newValue.equals("")) { - logger.warn("We received an empty rule for " + key + ", the current working rule is unknown, the empty rule will not take effect."); + logger.warn("an empty rule is received for " + key + ", the current working rule is unknown, " + + "the empty rule will not take effect."); return; } event.setChangeType(ConfigChangeType.MODIFIED); diff --git a/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/sources/ZooKeeperConfigurationSource.java b/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/sources/ZooKeeperConfigurationSource.java index 4366ac9..c7d5082 100644 --- a/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/sources/ZooKeeperConfigurationSource.java +++ b/dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/sources/ZooKeeperConfigurationSource.java @@ -45,7 +45,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; /** - * + * zookeeper archaius source. */ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, Closeable { public static final String ARCHAIUS_SOURCE_ADDRESS_KEY = "archaius.zk.address"; @@ -69,7 +69,8 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, private URL url; public ZooKeeperConfigurationSource(URL url) { - this(System.getProperty(ARCHAIUS_SOURCE_ADDRESS_KEY), 60 * 1000, 10000, System.getProperty(ARCHAIUS_CONFIG_ROOT_PATH_KEY, DEFAULT_CONFIG_ROOT_PATH)); + this(System.getProperty(ARCHAIUS_SOURCE_ADDRESS_KEY), 60 * 1000, 10000, + System.getProperty(ARCHAIUS_CONFIG_ROOT_PATH_KEY, DEFAULT_CONFIG_ROOT_PATH)); this.url = url; } @@ -86,7 +87,8 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, */ public ZooKeeperConfigurationSource(String connectString, int sessionTimeout, int connectTimeout, String configRootPath) { if (connectString == null) { - throw new IllegalArgumentException("connectString==null, must specify the address to connect for zookeeper archaius source."); + throw new IllegalArgumentException("connectString is null, must specify the address to connect for " + + "zookeeper archaius source."); } if (!configRootPath.startsWith("/")) { @@ -101,13 +103,16 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, if (!connected) { boolean check = Boolean.parseBoolean(System.getProperty(ARCHAIUS_CONFIG_CHECK_KEY, "false")); if (check) { - throw new IllegalStateException("Failed to connect to ConfigCenter Zookeeper : " + connectString + " in " + connectTimeout + "ms."); + throw new IllegalStateException("Failed to connect to config center (zookeeper): " + + connectString + " in " + connectTimeout + "ms."); } else { - logger.warn("Cannot connect to ConfigCenter at zookeeper " + connectString + " in " + connectTimeout + "ms"); + logger.warn("Cannot connect to config center (zookeeper) " + connectString + + " in " + connectTimeout + "ms"); } } } catch (InterruptedException e) { - throw new IllegalStateException("The thread was interrupted unexpectedly when try connecting to zookeeper " + connectString + " as ConfigCenter, ", e); + throw new IllegalStateException("The thread was interrupted unexpectedly when try connecting to zookeeper " + + connectString + " config center, ", e); } this.client = client; this.configRootPath = configRootPath; @@ -117,7 +122,7 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, /** * Creates the pathChildrenCache using the CuratorFramework client and ZK root path node for the config * - * @param client + * @param client zookeeper client * @param configRootPath path to ZK root parent node for the rest of the configuration properties (ie. /<my-app>/config) */ public ZooKeeperConfigurationSource(CuratorFramework client, String configRootPath) { @@ -127,15 +132,13 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, } /** - * Adds a listener to the pathChildrenCache, initializes the cache, then starts the cache-management background thread - * - * @throws Exception + * Adds a listener to the pathChildrenCache, initializes the cache, then starts the cache-management background + * thread */ public void start() throws Exception { - // create the watcher for future configuration updatess + // create the watcher for future configuration updates treeCache.getListenable().addListener(new TreeCacheListener() { - public void childEvent(CuratorFramework aClient, TreeCacheEvent event) - throws Exception { + public void childEvent(CuratorFramework aClient, TreeCacheEvent event) throws Exception { TreeCacheEvent.Type type = event.getType(); ChildData data = event.getData(); @@ -148,7 +151,9 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, return; } - // TODO We limit the notification of config changes to a specific path level, for example /dubbo/config/service/configurators, other config changes not in this level will not get notified, say /dubbo/config/dubbo.properties + // TODO We limit the notification of config changes to a specific path level, for example + // /dubbo/config/service/configurators, other config changes not in this level will not get notified, + // say /dubbo/config/dubbo.properties if (data.getPath().split("/").length == 5) { byte[] value = data.getData(); String stringValue = new String(value, charset); @@ -206,7 +211,8 @@ public class ZooKeeperConfigurationSource implements WatchedConfigurationSource, try { initializedLatch.await(); } catch (InterruptedException e) { - logger.error("Being interrupted unexpectedly when waiting zookeeper to initialize, the config data may not ready yet, be careful!"); + logger.error("Being interrupted unexpectedly when waiting zookeeper to initialize, the config data " + + "may not ready yet, be careful!"); } Map<String, ChildData> dataMap = treeCache.getCurrentChildren(configRootPath); diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryProtocol.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryProtocol.java index ca6dc2c..6bb4402 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryProtocol.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryProtocol.java @@ -581,11 +581,6 @@ public class RegistryProtocol implements Protocol { } notify(urls); } - - @Override - public URL getUrl() { - return subscribeUrl; - } } /**