Merge pull request apache#2810, code review and refactor for dubbo-configcenter.

Author: beiwei30

dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/RouterChain.java

@@ -51,7 +51,7 @@ public class RouterChain<T> {
 
     public static <T> RouterChain<T> buildChain(DynamicConfiguration dynamicConfiguration, URL url) {
         RouterChain<T> routerChain = new RouterChain<>(url);
-        List<RouterFactory> extensionFactories = ExtensionLoader.getExtensionLoader(RouterFactory.class).getActivateExtension(dynamicConfiguration.getUrl(), (String[]) null);
+        List<RouterFactory> extensionFactories = ExtensionLoader.getExtensionLoader(RouterFactory.class).getActivateExtension(url, (String[]) null);
         List<Router> routers = extensionFactories.stream()
                 .map(factory -> {
                     Router router = factory.getRouter(dynamicConfiguration, url);

dubbo-common/src/main/java/org/apache/dubbo/common/config/Configuration.java

@@ -17,7 +17,7 @@
 package org.apache.dubbo.common.config;
 
 /**
- *
+ * Configuration interface, to fetch the value for the specified key.
  */
 public interface Configuration {
     /**
@@ -58,6 +58,15 @@ public interface Configuration {
      */
     Object getProperty(String key);
 
+    /**
+     * Gets a property from the configuration. The default value will return if the configuration doesn't contain
+     * the mapping for the specified key.
+     *
+     * @param key property to retrieve
+     * @param defaultValue default value
+     * @return the value to which this configuration maps the specified key, or default value if the configuration
+     * contains no mapping for this key.
+     */
     Object getProperty(String key, Object defaultValue);
 
     /**

dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java

@@ -302,10 +302,6 @@ public Set<String> getLoadedExtensions() {
         return Collections.unmodifiableSet(new TreeSet<String>(cachedInstances.keySet()));
     }
 
-    public Set<Object> getLoadedExtensionInstances() {
-        return Collections.unmodifiableSet(cachedInstances.values().stream().map(Holder::get).collect(Collectors.toSet()));
-    }
-
     public Object getLoadedAdaptiveExtensionInstances() {
         return cachedAdaptiveInstance.get();
     }
@@ -340,6 +336,14 @@ public T getExtension(String name) {
         return (T) instance;
     }
 
+    /**
+     * Return all available extension instances.
+     */
+    public Set<T> getExtensions() {
+        return Collections.unmodifiableSet(getSupportedExtensions().stream().map(this::getExtension)
+                .collect(Collectors.toSet()));
+    }
+
     /**
      * Return default extension, return <code>null</code> if it's not configured.
      */

dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java

@@ -157,7 +157,7 @@ protected void checkRegistry() {
         // For compatibility purpose, use registry as the default config center if the registry protocol is zookeeper and there's no config center specified explicitly.
         RegistryConfig registry = registries.get(0);
         if (registry.isZookeeperProtocol()) {
-            Set<Object> loadedConfigurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getLoadedExtensionInstances();
+            Set<DynamicConfiguration> loadedConfigurations = ExtensionLoader.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();

dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ConfigCenterConfig.java

@@ -20,7 +20,6 @@
 import org.apache.dubbo.common.URL;
 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.StringUtils;
 import org.apache.dubbo.common.utils.UrlUtils;
 import org.apache.dubbo.config.support.Parameter;
@@ -31,7 +30,6 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
-import java.util.Set;
 
 /**
  *
@@ -102,16 +100,11 @@ private DynamicConfiguration startDynamicConfiguration() {
 //        checkConfigCenter();
 
         URL url = toConfigUrl();
-        Set<Object> loadedConfigurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getLoadedExtensionInstances();
-        if (CollectionUtils.isEmpty(loadedConfigurations)) {
-            DynamicConfiguration dynamicConfiguration = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getExtension(url.getProtocol());
-            // TODO, maybe we need a factory to do this?
-            dynamicConfiguration.setUrl(url);
-            dynamicConfiguration.init();
-            return dynamicConfiguration;
-        }
 
-        return (DynamicConfiguration) loadedConfigurations.iterator().next();
+        DynamicConfiguration dynamicConfiguration = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getExtension(url.getProtocol());
+        // TODO, maybe we need a factory to do this?
+        dynamicConfiguration.initWith(url);
+        return dynamicConfiguration;
     }
 
     private URL toConfigUrl() {

dubbo-configcenter/dubbo-configcenter-api/pom.xml

@@ -25,7 +25,7 @@
     <artifactId>dubbo-configcenter-api</artifactId>
     <packaging>jar</packaging>
     <name>${project.artifactId}</name>
-    <description>The api definition of the service configcenter module</description>
+    <description>The api definition of the service config-center module</description>
     <properties>
         <skip_maven_deploy>false</skip_maven_deploy>
     </properties>
@@ -37,4 +37,4 @@
             <version>${project.parent.version}</version>
         </dependency>
     </dependencies>
-</project>
+</project>

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/AbstractDynamicConfiguration.java

@@ -23,23 +23,36 @@
 import java.util.concurrent.ConcurrentMap;
 
 /**
+ * Dynamic configuration template class. The concrete implementation needs to provide implementation for three methods.
  *
+ * @see AbstractDynamicConfiguration#getTargetConfig(String, String, long)
+ * @see AbstractDynamicConfiguration#addListener(String, ConfigurationListener)
+ * @see AbstractDynamicConfiguration#createTargetListener(String, ConfigurationListener)
  */
-public abstract class AbstractDynamicConfiguration<TargetConfigListener> extends AbstractConfiguration implements DynamicConfiguration {
-    public static final String DEFAULT_GROUP = "dubbo";
+public abstract class AbstractDynamicConfiguration<TargetListener> extends AbstractConfiguration
+        implements DynamicConfiguration {
+    protected static final String DEFAULT_GROUP = "dubbo";
+
     protected URL url;
-    /**
-     * One key can register multiple target listeners, but one target listener only maps to one configuration listener
-     */
-    private ConcurrentMap<String, ConcurrentMap<ConfigurationListener, TargetConfigListener>> listenerToTargetListenerMap = new ConcurrentHashMap<>();
+
+    // One key can register multiple target listeners, but one target listener only maps to one configuration listener
+    private ConcurrentMap<String, ConcurrentMap<ConfigurationListener, TargetListener>> targetListeners =
+            new ConcurrentHashMap<>();
 
     public AbstractDynamicConfiguration() {
     }
 
+    @Override
+    public void initWith(URL url) {
+        this.url = url;
+    }
+
     @Override
     public void addListener(String key, ConfigurationListener listener) {
-        ConcurrentMap<ConfigurationListener, TargetConfigListener> listeners = listenerToTargetListenerMap.computeIfAbsent(key, k -> new ConcurrentHashMap<>());
-        TargetConfigListener targetListener = listeners.computeIfAbsent(listener, k -> createTargetConfigListener(key, listener));
+        ConcurrentMap<ConfigurationListener, TargetListener> listeners = targetListeners.computeIfAbsent(key,
+                k -> new ConcurrentHashMap<>());
+        TargetListener targetListener = listeners.computeIfAbsent(listener,
+                k -> createTargetListener(key, listener));
         addTargetListener(key, targetListener);
     }
 
@@ -60,33 +73,48 @@ public String getConfig(String key, ConfigurationListener listener) {
 
     @Override
     public String getConfig(String key, String group, ConfigurationListener listener) {
-        return getConfig(key, group, 0l, listener);
+        return getConfig(key, group, listener, 0L);
     }
 
     @Override
-    public String getConfig(String key, String group, long timeout, ConfigurationListener listener) {
+    public String getConfig(String key, String group, ConfigurationListener listener, long timeout) {
         try {
             if (listener != null) {
                 this.addListener(key, listener);
             }
-            return getInternalProperty(key, group, timeout);
+            return getTargetConfig(key, group, timeout);
         } catch (Exception e) {
             throw new IllegalStateException(e.getMessage(), e);
         }
     }
 
-    public URL getUrl() {
-        return url;
-    }
-
-    public void setUrl(URL url) {
-        this.url = url;
-    }
-
-    protected abstract String getInternalProperty(String key, String group, long timeout);
+    /**
+     * Fetch dynamic configuration from backend config storage. If timeout exceeds, exception should be thrown.
+     *
+     * @param key     property key
+     * @param group   group
+     * @param timeout timeout
+     * @return target config value
+     */
+    protected abstract String getTargetConfig(String key, String group, long timeout);
 
-    protected abstract void addTargetListener(String key, TargetConfigListener listener);
+    /**
+     * Register a native listener to the backend config storage so that Dubbo has chance to get notified when the
+     * value changes.
+     *
+     * @param key      property key listener is interested.
+     * @param listener native listener for the backend config storage
+     */
+    protected abstract void addTargetListener(String key, TargetListener listener);
 
-    protected abstract TargetConfigListener createTargetConfigListener(String key, ConfigurationListener listener);
+    /**
+     * Create a native listener for the backend config storage, eventually ConfigurationListener will get notified once
+     * the value changes.
+     *
+     * @param key      property key the native listener will listen on
+     * @param listener ConfigurationListener instance
+     * @return native listener for the backend config storage
+     */
+    protected abstract TargetListener createTargetListener(String key, ConfigurationListener listener);
 
 }

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigChangeEvent.java

@@ -17,7 +17,9 @@
 package org.apache.dubbo.configcenter;
 
 /**
+ * Config change event.
  *
+ * @see ConfigChangeType
  */
 public class ConfigChangeEvent {
     private String key;

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigChangeType.java

@@ -17,10 +17,21 @@
 package org.apache.dubbo.configcenter;
 
 /**
- *
+ * Config change event type
  */
 public enum ConfigChangeType {
+    /**
+     * A config is created.
+     */
     ADDED,
+
+    /**
+     * A config is updated.
+     */
     MODIFIED,
+
+    /**
+     * A config is deleted.
+     */
     DELETED
 }

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigType.java

@@ -17,9 +17,16 @@
 package org.apache.dubbo.configcenter;
 
 /**
- *
+ * Config type
  */
 public enum ConfigType {
+    /**
+     * For Dubbo dynamic config other than routing rules.
+     */
     CONFIGURATORS,
+
+    /**
+     * For Dubbo routing rules
+     */
     ROUTERS
 }

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java

@@ -19,11 +19,18 @@
 import org.apache.dubbo.common.URL;
 
 /**
- *
+ * Config listener, will get notified when the config it listens on changes.
  */
 public interface ConfigurationListener {
 
+    /**
+     * Listener call back method. Listener gets notified by this method once there's any change happens on the config
+     * the listener listens on.
+     *
+     * @param event config change event
+     */
     void process(ConfigChangeEvent event);
 
+    // FIXME: why we need this?
     URL getUrl();
 }

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationUtils.java

@@ -28,7 +28,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 /**
- *
+ * Utilities for manipulating configurations from different sources
  */
 public class ConfigurationUtils {
     private static final CompositeConfiguration compositeConfiguration;
@@ -64,17 +64,16 @@ public static CompositeConfiguration getRuntimeCompositeConf(URL url, String met
     }
 
     /**
-     * If user opens DynamicConfig, the extension instance must has been created during the initialization of ConfigCenterConfig with the right extension type user specified.
-     * If no DynamicConfig presents, NopDynamicConfiguration will be used.
-     *
-     * @return
+     * If user opens DynamicConfig, the extension instance must has been created during the initialization of
+     * ConfigCenterConfig with the right extension type user specified. If no DynamicConfig presents,
+     * NopDynamicConfiguration will be used.
      */
     public static DynamicConfiguration getDynamicConfiguration() {
-        Set<Object> configurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getLoadedExtensionInstances();
+        Set<DynamicConfiguration> configurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getExtensions();
         if (CollectionUtils.isEmpty(configurations)) {
             return ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getDefaultExtension();
         } else {
-            return (DynamicConfiguration) configurations.iterator().next();
+            return configurations.iterator().next();
         }
     }
 

dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationWrapper.java

@@ -20,7 +20,7 @@
 import org.apache.dubbo.common.config.Configuration;
 
 /**
- *
+ * A wrapper to fetch a config for the specific key with the different prefix in the specified order.
  */
 public class ConfigurationWrapper extends AbstractConfiguration {
     private String application;
@@ -36,6 +36,7 @@ public ConfigurationWrapper(String application, String service, String method, C
         this.delegate = configuration;
     }
 
+    // FIXME: I think the order is wrong, service.method.key go first, then service.key, and then application.key
     @Override
     protected Object getInternalProperty(String key) {
         Object value = delegate.getProperty(application + "." + key);