code review around RouterFactory

dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/AbstractRouterFactory.java → dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/CacheableRouterFactory.java

@@ -22,10 +22,10 @@
 import java.util.concurrent.ConcurrentMap;
 
 /**
- * If you want to provide a Router implementation based on design of v2.7.0, please extend from this abstract class.
- * For 2.6.x style Router, please implement and use RouterFactory directly.
+ * If you want to provide a router implementation based on design of v2.7.0, please extend from this abstract class.
+ * For 2.6.x style router, please implement and use RouterFactory directly.
  */
-public abstract class AbstractRouterFactory implements RouterFactory {
+public abstract class CacheableRouterFactory implements RouterFactory {
     private ConcurrentMap<String, Router> routerMap = new ConcurrentHashMap<>();
 
     @Override

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

@@ -33,7 +33,7 @@
  */
 public interface Router extends Comparable<Router> {
     /**
-     * get the router url.
+     * Get the router url.
      *
      * @return url
      */
@@ -63,24 +63,26 @@ default <T> void notify(List<Invoker<T>> invokers) {
     }
 
     /**
-     * To decide whether this router need to execute every time an RPC comes or should only execute when addresses or rule change.
+     * To decide whether this router need to execute every time an RPC comes or should only execute when addresses or
+     * rule change.
      *
-     * @return
+     * @return true if the router need to execute every time.
      */
     boolean isRuntime();
 
     /**
-     * To decide whether this router should take effect when none of the invoker can match the router rule, which means the {@link #route(List, URL, Invocation)} would be empty.
-     * Most of time, most router implementation would default this value to false.
+     * To decide whether this router should take effect when none of the invoker can match the router rule, which
+     * means the {@link #route(List, URL, Invocation)} would be empty. Most of time, most router implementation would
+     * default this value to false.
      *
-     * @return
+     * @return true to execute if none of invokers matches the current router
      */
     boolean isForce();
 
     /**
-     * used to sort routers.
+     * Router's priority, used to sort routers.
      *
-     * @return
+     * @return router's priority
      */
     int getPriority();
 }

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

@@ -27,13 +27,12 @@
 import java.util.stream.Collectors;
 
 /**
- *
+ * Router chain
  */
 public class RouterChain<T> {
 
     // full list of addresses from registry, classified by method name.
     private List<Invoker<T>> invokers = Collections.emptyList();
-    private URL url;
 
     // containing all routers, reconstruct every time 'route://' urls change.
     private volatile List<Router> routers = Collections.emptyList();
@@ -47,8 +46,6 @@ public static <T> RouterChain<T> buildChain(URL url) {
     }
 
     private RouterChain(URL url) {
-        this.url = url;
-
         List<RouterFactory> extensionFactories = ExtensionLoader.getExtensionLoader(RouterFactory.class)
                 .getActivateExtension(url, (String[]) null);
 
@@ -74,9 +71,10 @@ public void addRouter(Router router) {
     }
 
     /**
-     * If we use route:// protocol in version before 2.7.0, each URL will generate a Router instance,
-     * so we should keep the routers up to date, that is, each time router URLs changes, we should update the routers list,
-     * only keep the builtinRouters which are available all the time and the latest notified routers which are generated from URLs.
+     * If we use route:// protocol in version before 2.7.0, each URL will generate a Router instance, so we should
+     * keep the routers up to date, that is, each time router URLs changes, we should update the routers list, only
+     * keep the builtinRouters which are available all the time and the latest notified routers which are generated
+     * from URLs.
      *
      * @param routers routers from 'router://' rules in 2.6.x or before.
      */
@@ -87,9 +85,6 @@ public void addRouters(List<Router> routers) {
         newRouters.addAll(routers);
         this.routers = newRouters;
         this.sort();
-       /* if (invokers != null) {
-            this.rebuild(invokers, url, null);
-        }*/
     }
 
     private void sort() {
@@ -105,9 +100,6 @@ private void sort() {
     public List<Invoker<T>> route(URL url, Invocation invocation) {
         List<Invoker<T>> finalInvokers = invokers;
         for (Router router : routers) {
-//            if (router.isRuntime()) {
-//                finalInvokers = router.route(finalInvokers, url, invocation);
-//            }
             finalInvokers = router.route(finalInvokers, url, invocation);
         }
         return finalInvokers;

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

@@ -27,19 +27,20 @@
  *
  * @see org.apache.dubbo.rpc.cluster.Cluster#join(Directory)
  * @see org.apache.dubbo.rpc.cluster.Directory#list(org.apache.dubbo.rpc.Invocation)
- *
- * Note Router has a different behaviour since 2.7.0, for each type of Router, there will only has one Router instance for each service.
- * See {@link AbstractRouterFactory} and {@link RouterChain} for how to extend a new Router or how the Router instances are loaded.
+ * <p>
+ * Note Router has a different behaviour since 2.7.0, for each type of Router, there will only has one Router instance
+ * for each service. See {@link CacheableRouterFactory} and {@link RouterChain} for how to extend a new Router or how
+ * the Router instances are loaded.
  */
 @SPI
 public interface RouterFactory {
 
     /**
      * Create router.
-     * Since 2.7.0, most of the time, we will not use @Adaptive feature, so it's keeped only for compatibility.
+     * Since 2.7.0, most of the time, we will not use @Adaptive feature, so it's kept only for compatibility.
      *
-     * @param url
-     * @return router
+     * @param url url
+     * @return router instance
      */
     @Adaptive("protocol")
     Router getRouter(URL url);

dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/configurator/parser/ConfigParser.java

@@ -32,28 +32,29 @@
 import java.util.Map;
 
 /**
- *
+ * Config parser
  */
 public class ConfigParser {
 
     public static List<URL> parseConfigurators(String rawConfig) {
         List<URL> urls = new ArrayList<>();
-        ConfiguratorConfig configuratorConfig = parseObject(rawConfig, ConfiguratorConfig.class);
+        ConfiguratorConfig configuratorConfig = parseObject(rawConfig);
 
         String scope = configuratorConfig.getScope();
         List<ConfigItem> items = configuratorConfig.getConfigs();
 
         if (ConfiguratorConfig.SCOPE_APPLICATION.equals(scope)) {
             items.forEach(item -> urls.addAll(appItemToUrls(item, configuratorConfig)));
-        } else { // servcie scope by default.
+        } else {
+            // service scope by default.
             items.forEach(item -> urls.addAll(serviceItemToUrls(item, configuratorConfig)));
         }
         return urls;
     }
 
-    public static <T> T parseObject(String rawConfig, Class<T> clazz) {
-        Constructor constructor = new Constructor(clazz);
-        TypeDescription itemDescription = new TypeDescription(clazz);
+    private static <T> T parseObject(String rawConfig) {
+        Constructor constructor = new Constructor(ConfiguratorConfig.class);
+        TypeDescription itemDescription = new TypeDescription(ConfiguratorConfig.class);
         itemDescription.addPropertyParameters("items", ConfigItem.class);
         constructor.addTypeDescription(itemDescription);
 
@@ -63,13 +64,7 @@ public static <T> T parseObject(String rawConfig, Class<T> clazz) {
 
     private static List<URL> serviceItemToUrls(ConfigItem item, ConfiguratorConfig config) {
         List<URL> urls = new ArrayList<>();
-        List<String> addresses = item.getAddresses();
-        if (addresses == null) {
-            addresses = new ArrayList<>();
-        }
-        if (addresses.size() == 0) {
-            addresses.add(Constants.ANYHOST_VALUE);
-        }
+        List<String> addresses = parseAddresses(item);
 
         addresses.forEach(addr -> {
             StringBuilder urlBuilder = new StringBuilder();
@@ -78,20 +73,15 @@ private static List<URL> serviceItemToUrls(ConfigItem item, ConfiguratorConfig c
             urlBuilder.append(appendService(config.getKey()));
             urlBuilder.append(toParameterString(item));
 
-            urlBuilder.append("&enabled=");
-            if (item.getType() == null || ConfigItem.GENERAL_TYPE.equals(item.getType())) {
-                urlBuilder.append(config.getEnabled());
-            } else {
-                urlBuilder.append(item.getEnabled());
-            }
+            parseEnabled(item, config, urlBuilder);
 
             urlBuilder.append("&category=").append(Constants.DYNAMIC_CONFIGURATORS_CATEGORY);
             urlBuilder.append("&configVersion=").append(config.getConfigVersion());
 
             List<String> apps = item.getApplications();
             if (apps != null && apps.size() > 0) {
                 apps.forEach(app -> {
-                    urls.add(URL.valueOf(urlBuilder.append("&application=" + app).toString()));
+                    urls.add(URL.valueOf(urlBuilder.append("&application=").append(app).toString()));
                 });
             } else {
                 urls.add(URL.valueOf(urlBuilder.toString()));
@@ -103,13 +93,7 @@ private static List<URL> serviceItemToUrls(ConfigItem item, ConfiguratorConfig c
 
     private static List<URL> appItemToUrls(ConfigItem item, ConfiguratorConfig config) {
         List<URL> urls = new ArrayList<>();
-        List<String> addresses = item.getAddresses();
-        if (addresses == null) {
-            addresses = new ArrayList<>();
-        }
-        if (addresses.size() == 0) {
-            addresses.add(Constants.ANYHOST_VALUE);
-        }
+        List<String> addresses = parseAddresses(item);
         for (String addr : addresses) {
             StringBuilder urlBuilder = new StringBuilder();
             urlBuilder.append("override://").append(addr).append("/");
@@ -126,12 +110,7 @@ private static List<URL> appItemToUrls(ConfigItem item, ConfiguratorConfig confi
 
                 urlBuilder.append("&application=").append(config.getKey());
 
-                urlBuilder.append("&enabled=");
-                if (item.getType() == null || ConfigItem.GENERAL_TYPE.equals(item.getType())) {
-                    urlBuilder.append(config.getEnabled());
-                } else {
-                    urlBuilder.append(item.getEnabled());
-                }
+                parseEnabled(item, config, urlBuilder);
 
                 urlBuilder.append("&category=").append(Constants.APP_DYNAMIC_CONFIGURATORS_CATEGORY);
                 urlBuilder.append("&configVersion=").append(config.getConfigVersion());
@@ -152,7 +131,8 @@ private static String toParameterString(ConfigItem item) {
         }
         Map<String, String> parameters = item.getParameters();
         if (parameters == null || parameters.isEmpty()) {
-            throw new IllegalStateException("Invalid configurator rule, please specify at least one parameter you want to change in the rule!");
+            throw new IllegalStateException("Invalid configurator rule, please specify at least one parameter " +
+                    "you want to change in the rule.");
         }
 
         parameters.forEach((k, v) -> {
@@ -175,13 +155,14 @@ private static String toParameterString(ConfigItem item) {
     private static String appendService(String serviceKey) {
         StringBuilder sb = new StringBuilder();
         if (StringUtils.isEmpty(serviceKey)) {
-            throw new IllegalStateException("service field in coniguration is null!");
+            throw new IllegalStateException("service field in configuration is null.");
         }
+
         String interfaceName = serviceKey;
         int i = interfaceName.indexOf("/");
         if (i > 0) {
             sb.append("group=");
-            sb.append(interfaceName.substring(0, i));
+            sb.append(interfaceName, 0, i);
             sb.append("&");
 
             interfaceName = interfaceName.substring(i + 1);
@@ -198,4 +179,23 @@ private static String appendService(String serviceKey) {
         return sb.toString();
     }
 
+    private static void parseEnabled(ConfigItem item, ConfiguratorConfig config, StringBuilder urlBuilder) {
+        urlBuilder.append("&enabled=");
+        if (item.getType() == null || ConfigItem.GENERAL_TYPE.equals(item.getType())) {
+            urlBuilder.append(config.getEnabled());
+        } else {
+            urlBuilder.append(item.getEnabled());
+        }
+    }
+
+    private static List<String> parseAddresses(ConfigItem item) {
+        List<String> addresses = item.getAddresses();
+        if (addresses == null) {
+            addresses = new ArrayList<>();
+        }
+        if (addresses.size() == 0) {
+            addresses.add(Constants.ANYHOST_VALUE);
+        }
+        return addresses;
+    }
 }

dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java

@@ -26,7 +26,6 @@
 public abstract class AbstractRouter implements Router {
     protected int priority;
     protected boolean force = false;
-    protected boolean enabled = true;
     protected URL url;
 
     protected DynamicConfiguration configuration;
@@ -71,14 +70,6 @@ public int compareTo(Router o) {
         return (this.getPriority() >= o.getPriority()) ? 1 : -1;
     }
 
-    public boolean isEnabled() {
-        return enabled;
-    }
-
-    public void setEnabled(boolean enabled) {
-        this.enabled = enabled;
-    }
-
     public int getPriority() {
         return priority;
     }

dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/condition/ConditionRouter.java

@@ -44,21 +44,26 @@
  *
  */
 public class ConditionRouter extends AbstractRouter implements Comparable<Router> {
-    public static final String NAME = "CONDITION_ROUTER";
+    public static final String NAME = "condition";
+
     private static final Logger logger = LoggerFactory.getLogger(ConditionRouter.class);
     protected static Pattern ROUTE_PATTERN = Pattern.compile("([&!=,]*)\\s*([^&!=,\\s]+)");
     protected Map<String, MatchPair> whenCondition;
     protected Map<String, MatchPair> thenCondition;
 
-    public ConditionRouter(String rule, boolean force) {
+    private boolean enabled;
+
+    public ConditionRouter(String rule, boolean force, boolean enabled) {
         this.force = force;
+        this.enabled = enabled;
         this.init(rule);
     }
 
     public ConditionRouter(URL url) {
         this.url = url;
         this.priority = url.getParameter(Constants.PRIORITY_KEY, 0);
         this.force = url.getParameter(Constants.FORCE_KEY, false);
+        this.enabled = url.getParameter(Constants.ENABLED_KEY, true);
         init(url.getParameterAndDecoded(Constants.RULE_KEY));
     }
 
@@ -154,7 +159,7 @@ else if (",".equals(separator)) { // Should be separated by ','
     @Override
     public <T> List<Invoker<T>> route(List<Invoker<T>> invokers, URL url, Invocation invocation)
             throws RpcException {
-        if (!isEnabled()) {
+        if (!enabled) {
             return invokers;
         }
 
@@ -194,11 +199,6 @@ public boolean isRuntime() {
         return this.url.getParameter(Constants.RUNTIME_KEY, false);
     }
 
-    @Override
-    public boolean isEnabled() {
-        return url == null ? enabled : url.getParameter(Constants.ENABLED_KEY, true);
-    }
-
     @Override
     public URL getUrl() {
         return url;