Adding Consul as the Service Discovery and Configuration Center #4647

felixgis14 · 2024-12-27T02:37:11Z

Add the registry-consul submodule to the service-registry module to register and discover Consul services.
Add the config-consul submodule under the dynamic-config module to implement the key-value of the consul as the configuration center.
Add demo-consul to the demo module to test the Consul service registration, discovery, and configuration center.

liubao68 · 2024-12-27T06:22:29Z

dynamic-config/config-consul/pom.xml

+      <artifactId>java-chassis-core</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework.boot</groupId>


Why need this starter?

During development, I found that I could not use the @slf4j annotation to add logs. Later, I used the Logger method. I forgot to delete this dependency.

liubao68 · 2024-12-27T06:40:31Z

service-registry/pom.xml

    <module>registry-nacos</module>
    <module>registry-zookeeper</module>
    <module>registry-etcd</module>
+	<module>registry-consul</module>


liubao68 · 2024-12-27T06:48:42Z

demo/demo-consul/consumer/pom.xml

+    <dependency>
+      <groupId>com.google.protobuf</groupId>
+      <artifactId>protobuf-java</artifactId>
+      <version>3.25.5</version>


Is it ptotobuf-java needed?

Dependencies copied from other modules have been deleted

checkstyle,rat_check

linelint check

liubao68 · 2024-12-31T01:47:05Z

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

+    }
+  }
+
+  @Resource


It is constructor injection. Maybe it's not necessary for @Resource

liubao68 · 2024-12-31T01:53:03Z

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

+        consulConfig.getInstanceTag());
+
+    runTask(tagData, path);
+    this.tagData = parseData(path);


Is this line necessary? runTask has already updated tagData

liubao68 · 2024-12-31T01:54:50Z

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

+  }
+
+  private void runTask(Map<String, Object> serviceData, String path) {
+    watchFuture = taskScheduler.scheduleWithFixedDelay(new GetDataRunnable(serviceData, this, path),


It's better to poll all type of configs in each job, not each type a job.

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

liubao68 · 2024-12-31T02:01:26Z

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

+    try {
+      consulConfigClient = consulConfigClient(environment);
+      ConsulConfigClientClear consulConfigClientClear = consulConfigClear();
+      LOGGER.info("consulConfigClientClear getPhase:{}", consulConfigClientClear.getPhase());


ConsulConfigClientClear can be deleted? Seems no use. And this code slip will create many ConsulConfigClient instance and is not correct

liubao68 · 2024-12-31T02:02:05Z

service-registry/pom.xml

    <module>registry-nacos</module>
    <module>registry-zookeeper</module>
    <module>registry-etcd</module>
+	  <module>registry-consul</module>


liubao68 · 2024-12-31T02:06:57Z

...ry/registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulDiscovery.java

+    scheduledFuture = taskScheduler.scheduleWithFixedDelay(
+        () -> instanceChangedListener.onInstanceChanged(
+            name(), BootStrapProperties.readApplication(environment), serviceName, getInstances(serviceName, new QueryParams(consulDiscoveryProperties.getConsistencyMode()))
+        ), Duration.ofMillis(consulDiscoveryProperties.getDelayTime())


call onInstanceChanged must base on instance list already changed. Make sure if consul can indication instance change.

Refactored service registration, service discovery, and configuration center using the new consul client

…va-chassis

Refactored service registration, service discovery, and configuration center using the new consul client

liubao68 · 2025-01-02T06:22:01Z

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

+  @Bean
+  @ConditionalOnBean(value = {Consul.class, Environment.class, RegistrationId.class, DataCenterProperties.class})
+  @ConditionalOnMissingBean
+  public ConsulDiscovery etcdDiscovery() {


liubao68 · 2025-01-02T06:22:37Z

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

+import java.util.concurrent.ConcurrentHashMap;
+
+public class UpdateHandler {
+  private final Map<String, Object> valueCache = new ConcurrentHashMap<>();


valueCache never used

liubao68 · 2025-01-02T06:22:57Z

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

+public class ConsulDynamicPropertiesSource implements DynamicPropertiesSource {
+  public static final String SOURCE_NAME = "consul";
+
+  private final Map<String, Object> valueCache = new ConcurrentHashMap<>();


valueCache never updated

liubao68 · 2025-01-02T06:25:53Z

...ry/registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulDiscovery.java

+    if (!CollectionUtils.isEmpty(healthServices)) {
+      for (ServiceHealth serviceHealth : healthServices) {
+        Service service = serviceHealth.getService();
+        logger.info("healthService:{}", service.getId());


Do not print too many logs

liubao68 · 2025-01-02T06:27:33Z

...ry/registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulDiscovery.java

+
+public class ConsulDiscovery implements Discovery<ConsulDiscoveryInstance> {
+
+  private static final Logger logger = LoggerFactory.getLogger(ConsulDiscovery.class);


using LOGGER

liubao68 · 2025-01-02T06:30:40Z

...ry/registry-consul/src/main/java/org/apache/servicecomb/registry/consul/utils/InetUtils.java

+    Future<String> result = this.executorService.submit(address::getHostName);
+
+    String hostname;
+    try {
+      hostname = result.get(1, TimeUnit.SECONDS);
+    } catch (Exception e) {
+      hostname = "localhost";
+    }


Why create a new thread to get host name?

Use EndpointUtils to obtain the IP address and port number.

liubao68 · 2025-01-02T06:34:52Z

...registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulRegistration.java

+    String instanceId = registrationId.getInstanceId();
+    String serviceName = BootStrapProperties.readServiceName(environment);
+    consulInstance = new ConsulInstance();
+    consulInstance.setServiceId(serviceName + "-" + serverPort);


Can serviceName be serviceId? Why add port?

This is a problem. In the case of multiple instances, IP addresses and ports are used to identify different services.

liubao68 · 2025-01-02T06:36:16Z

...registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulRegistration.java

+    String serverPort;
+    if (restAddress.contains("?")) {
+      serverPort = restAddress.substring(restAddress.indexOf(":") + 1, restAddress.indexOf("?"));
+    } else {


Is serverPort required?

This is a problem. In the case of multiple instances, IP addresses and ports are used to identify different services.

liubao68 · 2025-01-02T06:39:04Z

...registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulRegistration.java

+    meta.put("serviceName", consulInstance.getServiceName());
+    meta.put("version", consulInstance.getVersion());
+    meta.put("instanceId", registrationId.getInstanceId());
+    meta.put("env", consulInstance.getEnvironment());
+    meta.put("application", consulInstance.getApplication());
+    meta.put("alias", consulInstance.getAlias() != null ? consulInstance.getAlias() : "");
+    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
+    meta.put("endpoints", gson.toJson(consulInstance.getEndpoints()));


Maybe you can add consulInstance as a json propertly, and code can be simpler. See Zookeeper for an example.

liubao68 · 2025-01-02T06:43:41Z

...registry-consul/src/main/java/org/apache/servicecomb/registry/consul/ConsulRegistration.java

+    registrationBuilder = ImmutableRegistration.builder()
+        .id(consulInstance.getServiceId())
+        .name(consulInstance.getServiceName())
+        .address(consulDiscoveryProperties.getIpAddress())


See zookeeper/nacos implementation how to get ip address and port. Do not use the new InetUtils added. InetUtils implementation is not correct.

Service id Service information storage valueCache usage

Adding Consul as the Service Discovery and Configuration Center

aa09508

liubao68 reviewed Dec 27, 2024

View reviewed changes

flygoods and others added 2 commits December 27, 2024 17:56

style: Fix the problems detected by CI

8d51702

checkstyle,rat_check

style: Fix the problems detected by CI

b0523f6

linelint check

liubao68 reviewed Dec 31, 2024

View reviewed changes

...consul/src/main/java/org/apache/servicecomb/config/consul/ConsulDynamicPropertiesSource.java Outdated Show resolved Hide resolved

liubao68 reviewed Dec 31, 2024

View reviewed changes

felixgis14 and others added 5 commits January 1, 2025 00:35

refactor: Refactored code

2120705

Refactored service registration, service discovery, and configuration center using the new consul client

Merge branch 'apache:master' into master

ef3c117

Merge branch 'master' of https://github.com/felixgis14/servicecomb-ja…

0859505

…va-chassis

refactor: Refactored code

6f88f88

Refactored service registration, service discovery, and configuration center using the new consul client

Fix pom.xml indention

f43ca34

liubao68 reviewed Jan 2, 2025

View reviewed changes

refactor: Optimize codes found in code review

310add6

Service id Service information storage valueCache usage

liubao68 merged commit 2fc6d40 into apache:master Jan 3, 2025
6 checks passed


		public class ConsulDiscovery implements Discovery<ConsulDiscoveryInstance> {

		private static final Logger logger = LoggerFactory.getLogger(ConsulDiscovery.class);

Adding Consul as the Service Discovery and Configuration Center #4647

Adding Consul as the Service Discovery and Configuration Center #4647

Uh oh!

Conversation

felixgis14 commented Dec 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants