refactor: Optimize codes found in code review

Service id
Service information storage
valueCache usage

Author: felixgis14

dynamic-config/config-consul/src/main/java/org/apache/servicecomb/config/consul/ConsulConfigClient.java

@@ -47,7 +47,7 @@
 import static org.apache.servicecomb.config.consul.ConsulConfig.PATH_VERSION;
 
 public class ConsulConfigClient {
-  private static final Logger logger = LoggerFactory.getLogger(ConsulConfigClient.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(ConsulConfigClient.class);
 
   public class GetDataRunnable implements Runnable {
 
@@ -81,7 +81,7 @@ public void run() {
     }
   }
 
-  private UpdateHandler updateHandler;
+  private ConsulDynamicPropertiesSource.UpdateHandler updateHandler;
 
   private ConsulConfig consulConfig;
 
@@ -107,7 +107,7 @@ public void run() {
 
   private Map<String, Object> allLast = new HashMap<>();
 
-  public ConsulConfigClient(UpdateHandler updateHandler, Environment environment, ConsulConfigProperties consulConfigProperties, Consul consulClient) {
+  public ConsulConfigClient(ConsulDynamicPropertiesSource.UpdateHandler updateHandler, Environment environment, ConsulConfigProperties consulConfigProperties, Consul consulClient) {
     this.updateHandler = updateHandler;
     this.consulConfig = new ConsulConfig(environment);
     this.environment = environment;
@@ -240,7 +240,7 @@ public Map<String, Object> parseData(String path) {
     try {
       return getValues(path);
     } catch (Exception e) {
-      logger.error(e.getMessage(), e);
+      LOGGER.error(e.getMessage(), e);
     }
     return new HashMap<>();
   }
@@ -266,7 +266,7 @@ private Map<String, Object> getValues(String path) {
       try {
         properties.load(new StringReader(decodedValue));
       } catch (IOException e) {
-        logger.error(e.getMessage(), e);
+        LOGGER.error(e.getMessage(), e);
       }
       values.putAll(toMap(properties));
     } else {

dynamic-config/config-consul/src/main/java/org/apache/servicecomb/config/consul/ConsulDynamicPropertiesSource.java

@@ -19,8 +19,12 @@
 
 import com.google.common.net.HostAndPort;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.servicecomb.config.ConfigurationChangedEvent;
 import org.apache.servicecomb.config.DynamicPropertiesSource;
+import org.apache.servicecomb.foundation.common.event.EventManager;
 import org.kiwiproject.consul.Consul;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.core.env.Environment;
 import org.springframework.core.env.MapPropertySource;
 import org.springframework.core.env.PropertySource;
@@ -29,6 +33,8 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 public class ConsulDynamicPropertiesSource implements DynamicPropertiesSource {
+  private static final Logger LOGGER = LoggerFactory.getLogger(ConsulDynamicPropertiesSource.class);
+
   public static final String SOURCE_NAME = "consul";
 
   private final Map<String, Object> valueCache = new ConcurrentHashMap<>();
@@ -38,6 +44,18 @@ public class ConsulDynamicPropertiesSource implements DynamicPropertiesSource {
   public ConsulDynamicPropertiesSource() {
   }
 
+  private final UpdateHandler updateHandler = new UpdateHandler();
+  public class UpdateHandler {
+    public void handle(Map<String, Object> current, Map<String, Object> last) {
+      ConfigurationChangedEvent event = ConfigurationChangedEvent.createIncremental(current, last);
+      LOGGER.info("Dynamic configuration changed: {}", event.getChanged());
+      valueCache.putAll(event.getAdded());
+      valueCache.putAll(event.getUpdated());
+      event.getDeleted().forEach((k, v) -> valueCache.remove(k));
+      EventManager.post(event);
+    }
+  }
+
   private ConsulConfigProperties consulConfigProperties(Environment environment) {
     ConsulConfig consulConfig = new ConsulConfig(environment);
     ConsulConfigProperties consulConfigProperties = new ConsulConfigProperties();
@@ -60,7 +78,7 @@ private Consul consulClient(ConsulConfigProperties consulConfigProperties) {
   private ConsulConfigClient consulConfigClient(Environment environment) {
     ConsulConfigProperties consulConfigProperties = consulConfigProperties(environment);
     Consul consulClient = consulClient(consulConfigProperties);
-    return new ConsulConfigClient(new UpdateHandler(), environment, consulConfigProperties, consulClient);
+    return new ConsulConfigClient(updateHandler, environment, consulConfigProperties, consulClient);
   }
 
   @Override

dynamic-config/config-consul/src/main/java/org/apache/servicecomb/config/consul/UpdateHandler.java

@@ -23,7 +23,6 @@
 import org.apache.servicecomb.registry.RegistrationId;
 import org.apache.servicecomb.registry.consul.config.ConsulDiscoveryProperties;
 import org.apache.servicecomb.registry.consul.config.ConsulProperties;
-import org.apache.servicecomb.registry.consul.utils.InetUtils;
 import org.kiwiproject.consul.Consul;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnBean;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
@@ -48,7 +47,7 @@ public ConsulProperties consulProperties() {
   @ConfigurationProperties(prefix = ConsulConst.CONSUL_DISCOVERY_REGISTRY_PREFIX)
   @ConditionalOnMissingBean
   public ConsulDiscoveryProperties consulDiscoveryProperties() {
-    return new ConsulDiscoveryProperties(new InetUtils());
+    return new ConsulDiscoveryProperties();
   }
 
   @Bean
@@ -65,7 +64,7 @@ public Consul consulClient(ConsulProperties consulProperties, ConsulDiscoveryPro
   @Bean
   @ConditionalOnBean(value = {Consul.class, Environment.class, RegistrationId.class, DataCenterProperties.class})
   @ConditionalOnMissingBean
-  public ConsulDiscovery etcdDiscovery() {
+  public ConsulDiscovery consulDiscovery() {
     return new ConsulDiscovery();
   }
 

service-registry/registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulConfiguration.java

@@ -20,10 +20,8 @@
 import com.google.common.collect.Lists;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
-import com.google.gson.reflect.TypeToken;
 import jakarta.annotation.Resource;
 import jakarta.validation.constraints.NotNull;
-import org.apache.commons.lang3.StringUtils;
 import org.apache.servicecomb.config.BootStrapProperties;
 import org.apache.servicecomb.registry.api.Discovery;
 import org.apache.servicecomb.registry.consul.config.ConsulDiscoveryProperties;
@@ -37,18 +35,16 @@
 import org.kiwiproject.consul.option.Options;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.core.env.Environment;
 import org.springframework.util.CollectionUtils;
 
-import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
 public class ConsulDiscovery implements Discovery<ConsulDiscoveryInstance> {
 
-  private static final Logger logger = LoggerFactory.getLogger(ConsulDiscovery.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(ConsulDiscovery.class);
 
   @Resource
   private ConsulProperties consulProperties;
@@ -62,11 +58,6 @@ public class ConsulDiscovery implements Discovery<ConsulDiscoveryInstance> {
   @Resource
   private Environment environment;
 
-  @Value("${servicecomb.rest.address:127.0.0.1:8080}")
-  private String restAddress;
-
-  private String serverPort = "";
-
   private List<ConsulDiscoveryInstance> consulDiscoveryInstanceList;
 
   private InstanceChangedListener<ConsulDiscoveryInstance> instanceChangedListener;
@@ -85,14 +76,14 @@ public boolean enabled(String application, String serviceName) {
 
   @Override
   public List<ConsulDiscoveryInstance> findServiceInstances(String application, String serviceName) {
-    logger.info("findServiceInstances application:{}, serviceName:{}", application, serviceName);
+    LOGGER.info("findServiceInstances application:{}, serviceName:{}", application, serviceName);
     consulDiscoveryInstanceList = getInstances(serviceName);
     return consulDiscoveryInstanceList;
   }
 
   @Override
   public List<String> findServices(String application) {
-    logger.info("ConsulDiscovery findServices(application={})", application);
+    LOGGER.info("ConsulDiscovery findServices(application={})", application);
     Map<String, Service> services = consulClient.agentClient().getServices();
     return Lists.newArrayList(services.keySet());
   }
@@ -109,17 +100,12 @@ public boolean enabled() {
 
   @Override
   public void init() {
-    logger.info("ConsulDiscovery init");
-    if (restAddress.contains("?")) {
-      serverPort = restAddress.substring(restAddress.indexOf(":") + 1, restAddress.indexOf("?"));
-    } else {
-      serverPort = restAddress.substring(restAddress.indexOf(":") + 1);
-    }
+    LOGGER.info("ConsulDiscovery init");
   }
 
   @Override
   public void run() {
-    logger.info("ConsulDiscovery run");
+    LOGGER.info("ConsulDiscovery run");
     String serviceName = BootStrapProperties.readServiceName(environment);
     HealthClient healthClient = consulClient.healthClient();
     svHealth = ServiceHealthCache.newCache(healthClient, serviceName, true, Options.BLANK_QUERY_OPTIONS, consulDiscoveryProperties.getWatchSeconds());
@@ -133,11 +119,10 @@ public void destroy() {
     if (svHealth != null) {
       svHealth.stop();
     }
-    String serviceName = BootStrapProperties.readServiceName(environment);
-    String serviceId = serviceName + "-" + serverPort;
-    logger.info("ConsulDiscovery destroy consul service id={}", serviceId);
+    String serviceId = consulDiscoveryProperties.getServiceId();
+    LOGGER.info("ConsulDiscovery destroy consul service id={}", serviceId);
     if (consulClient != null) {
-      logger.info("ConsulDiscovery consulClient destroy");
+      LOGGER.info("ConsulDiscovery consulClient destroy");
       consulClient.agentClient().deregister(serviceId);
       consulClient.destroy();
     }
@@ -154,24 +139,8 @@ private void addInstancesToList(List<ConsulDiscoveryInstance> instances, String
     if (!CollectionUtils.isEmpty(healthServices)) {
       for (ServiceHealth serviceHealth : healthServices) {
         Service service = serviceHealth.getService();
-        logger.info("healthService:{}", service.getId());
-        Map<String, String> meta = service.getMeta();
-        ConsulInstance consulInstance = new ConsulInstance();
-        consulInstance.setServiceId(service.getId());
-        consulInstance.setServiceName(meta.get("serviceName"));
-        consulInstance.setInstanceId(meta.get("instanceId"));
-        consulInstance.setApplication(meta.get("application"));
-        consulInstance.setEnvironment(meta.get("env"));
-        consulInstance.setAlias(meta.get("alias"));
-        consulInstance.setVersion(meta.get("version"));
         Gson gson = new GsonBuilder().disableHtmlEscaping().create();
-        String[] endpoints = gson.fromJson(meta.get("endpoints"), String[].class);
-        consulInstance.setEndpoints(Lists.newArrayList(endpoints));
-        if (StringUtils.isNotBlank(meta.get("properties"))) {
-          Type type = new TypeToken<Map<String, String>>() {
-          }.getType();
-          consulInstance.setProperties(gson.fromJson(meta.get("properties"), type));
-        }
+        ConsulInstance consulInstance = gson.fromJson(service.getMeta().get("meta"), ConsulInstance.class);
         instances.add(new ConsulDiscoveryInstance(consulInstance));
       }
     }