Improve ClusterServerConfigManager in sentinel-cluster-server-default and add basic test cases

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

sentinel-cluster/sentinel-cluster-server-default/src/main/java/com/alibaba/csp/sentinel/cluster/server/config/ClusterServerConfigManager.java

@@ -35,6 +35,7 @@ import com.alibaba.csp.sentinel.log.RecordLog;
 import com.alibaba.csp.sentinel.property.DynamicSentinelProperty;
 import com.alibaba.csp.sentinel.property.PropertyListener;
 import com.alibaba.csp.sentinel.property.SentinelProperty;
+import com.alibaba.csp.sentinel.slots.block.flow.FlowRuleUtil;
 import com.alibaba.csp.sentinel.util.AssertUtil;
 
 /**
@@ -95,7 +96,13 @@ public final class ClusterServerConfigManager {
         namespaceSetProperty.addListener(NAMESPACE_SET_PROPERTY_LISTENER);
     }
 
+    /**
+     * Register cluster server namespace set dynamic property.
+     *
+     * @param property server namespace set dynamic property
+     */
     public static void registerNamespaceSetProperty(SentinelProperty<Set<String>> property) {
+        AssertUtil.notNull(property, "namespace set dynamic property cannot be null");
         synchronized (NAMESPACE_SET_PROPERTY_LISTENER) {
             RecordLog.info(
                 "[ClusterServerConfigManager] Registering new namespace set dynamic property to Sentinel server "
@@ -106,7 +113,13 @@ public final class ClusterServerConfigManager {
         }
     }
 
+    /**
+     * Register cluster server transport configuration dynamic property.
+     *
+     * @param property server transport configuration dynamic property
+     */
     public static void registerServerTransportProperty(SentinelProperty<ServerTransportConfig> property) {
+        AssertUtil.notNull(property, "cluster server transport config dynamic property cannot be null");
         synchronized (TRANSPORT_PROPERTY_LISTENER) {
             RecordLog.info(
                 "[ClusterServerConfigManager] Registering new server transport dynamic property to Sentinel server "
@@ -117,14 +130,46 @@ public final class ClusterServerConfigManager {
         }
     }
 
+    /**
+     * Register cluster server global statistic (flow) configuration dynamic property.
+     *
+     * @param property server flow configuration dynamic property
+     */
+    public static void registerGlobalServerFlowProperty(SentinelProperty<ServerFlowConfig> property) {
+        AssertUtil.notNull(property, "cluster server flow config dynamic property cannot be null");
+        synchronized (GLOBAL_FLOW_PROPERTY_LISTENER) {
+            RecordLog.info(
+                "[ClusterServerConfigManager] Registering new server global flow dynamic property "
+                    + "to Sentinel server config manager");
+            globalFlowProperty.removeListener(GLOBAL_FLOW_PROPERTY_LISTENER);
+            property.addListener(GLOBAL_FLOW_PROPERTY_LISTENER);
+            globalFlowProperty = property;
+        }
+    }
+
+    /**
+     * Load provided server namespace set to property in memory.
+     *
+     * @param namespaceSet valid namespace set
+     */
     public static void loadServerNamespaceSet(Set<String> namespaceSet) {
         namespaceSetProperty.updateValue(namespaceSet);
     }
 
+    /**
+     * Load provided server transport configuration to property in memory.
+     *
+     * @param config valid cluster server transport configuration
+     */
     public static void loadGlobalTransportConfig(ServerTransportConfig config) {
         transportConfigProperty.updateValue(config);
     }
 
+    /**
+     * Load provided server global statistic (flow) configuration to property in memory.
+     *
+     * @param config valid cluster server flow configuration for global
+     */
     public static void loadGlobalFlowConfig(ServerFlowConfig config) {
         globalFlowProperty.updateValue(config);
     }
@@ -133,7 +178,7 @@ public final class ClusterServerConfigManager {
      * Load server flow config for a specific namespace.
      *
      * @param namespace a valid namespace
-     * @param config valid flow config for the namespace
+     * @param config    valid flow config for the namespace
      */
     public static void loadFlowConfig(String namespace, ServerFlowConfig config) {
         AssertUtil.notEmpty(namespace, "namespace cannot be empty");
@@ -141,6 +186,12 @@ public final class ClusterServerConfigManager {
         globalFlowProperty.updateValue(config);
     }
 
+    /**
+     * Add a transport config observer. The observers will be called as soon as
+     * there are some changes in transport config (e.g. token server port).
+     *
+     * @param observer a valid transport config observer
+     */
     public static void addTransportConfigChangeObserver(ServerTransportConfigObserver observer) {
         AssertUtil.notNull(observer, "observer cannot be null");
         TRANSPORT_CONFIG_OBSERVERS.add(observer);
@@ -175,16 +226,20 @@ public final class ClusterServerConfigManager {
         }
 
         newSet = new HashSet<>(newSet);
+        // Always add the `default` namespace to the namespace set.
         newSet.add(ServerConstants.DEFAULT_NAMESPACE);
+
         if (embedded) {
             // In embedded server mode, the server itself is also a part of service,
             // so it should be added to namespace set.
+            // By default, the added namespace is the appName.
             newSet.add(ConfigSupplierRegistry.getNamespaceSupplier().get());
         }
 
         Set<String> oldSet = ClusterServerConfigManager.namespaceSet;
         if (oldSet != null && !oldSet.isEmpty()) {
             for (String ns : oldSet) {
+                // Remove the cluster rule property for deprecated namespace set.
                 if (!newSet.contains(ns)) {
                     ClusterFlowRuleManager.removeProperty(ns);
                     ClusterParamFlowRuleManager.removeProperty(ns);
@@ -194,8 +249,10 @@ public final class ClusterServerConfigManager {
 
         ClusterServerConfigManager.namespaceSet = newSet;
         for (String ns : newSet) {
+            // Register the rule property if needed.
             ClusterFlowRuleManager.registerPropertyIfAbsent(ns);
             ClusterParamFlowRuleManager.registerPropertyIfAbsent(ns);
+            // Initialize the global QPS limiter for the namespace.
             GlobalRequestLimiter.initIfAbsent(ns);
         }
     }
@@ -294,7 +351,8 @@ public final class ClusterServerConfigManager {
 
     public static boolean isValidFlowConfig(ServerFlowConfig config) {
         return config != null && config.getMaxOccupyRatio() >= 0 && config.getExceedCount() >= 0
-            && config.getMaxAllowedQps() >= 0;
+            && config.getMaxAllowedQps() >= 0
+            && FlowRuleUtil.isWindowConfigValid(config.getSampleCount(), config.getIntervalMs());
     }
 
     public static double getExceedCount(String namespace) {
@@ -388,6 +446,15 @@ public final class ClusterServerConfigManager {
         return embedded;
     }
 
+    /**
+     * <p>Set the embedded mode flag for the token server. </p>
+     * <p>
+     * NOTE: developers SHOULD NOT manually invoke this method.
+     * The embedded flag should be initialized by Sentinel when starting token server.
+     * </p>
+     *
+     * @param embedded whether the token server is currently running in embedded mode
+     */
     public static void setEmbedded(boolean embedded) {
         ClusterServerConfigManager.embedded = embedded;
     }

sentinel-cluster/sentinel-cluster-server-default/src/test/java/com/alibaba/csp/sentinel/cluster/server/config/ClusterServerConfigManagerTest.java

@@ -0,0 +1,31 @@
+package com.alibaba.csp.sentinel.cluster.server.config;
+
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author Eric Zhao
+ */
+public class ClusterServerConfigManagerTest {
+
+    @Test
+    public void testIsValidTransportConfig() {
+        ServerTransportConfig badConfig1 = new ServerTransportConfig().setPort(-1);
+        ServerTransportConfig badConfig2 = new ServerTransportConfig().setPort(886622);
+        ServerTransportConfig goodConfig1 = new ServerTransportConfig().setPort(23456);
+        assertFalse(ClusterServerConfigManager.isValidTransportConfig(badConfig1));
+        assertFalse(ClusterServerConfigManager.isValidTransportConfig(badConfig2));
+        assertTrue(ClusterServerConfigManager.isValidTransportConfig(goodConfig1));
+    }
+
+    @Test
+    public void testIsValidFlowConfig() {
+        ServerFlowConfig badConfig1 = new ServerFlowConfig().setMaxAllowedQps(-2);
+        ServerFlowConfig badConfig2 = new ServerFlowConfig().setIntervalMs(1000).setSampleCount(3);
+        ServerFlowConfig badConfig3 = new ServerFlowConfig().setIntervalMs(1000).setSampleCount(0);
+        assertFalse(ClusterServerConfigManager.isValidFlowConfig(badConfig1));
+        assertFalse(ClusterServerConfigManager.isValidFlowConfig(badConfig2));
+        assertFalse(ClusterServerConfigManager.isValidFlowConfig(badConfig3));
+    }
+}