Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize dynamic config: integrate Zookeeper & Nacos, support interface-level dynamic config #1430

Merged
merged 9 commits into from
Oct 31, 2024
+1,793 −40
Merged

Optimize dynamic config: integrate Zookeeper & Nacos, support interface-level dynamic config #1430

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
528d0fb
integrate Zookeeper and Nacos as configuration centers
Narzisss Jul 21, 2024
853ed3f
support dynamic config at the interface level
Narzisss Sep 23, 2024
92351f2
Optimize interface-level dynamic config
Narzisss Sep 29, 2024
d841de9
Merge branch 'sofastack:master' into master
Narzisss Oct 15, 2024
7a354b3
Modify dynamic config test
Narzisss Oct 15, 2024
b8c60ae
Add DynamicUrl
Narzisss Oct 29, 2024
08a0a85
Optimize config update process
Narzisss Oct 30, 2024
0f613cf
Modify config update process
Narzisss Oct 31, 2024
117b33c
Modify ApolloDynamicConfigManagerTest
Narzisss Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions all/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,16 @@
<artifactId>sofa-rpc-config-apollo</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>com.alipay.sofa</groupId>
<artifactId>sofa-rpc-config-zk</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>com.alipay.sofa</groupId>
<artifactId>sofa-rpc-config-nacos</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>com.alipay.sofa</groupId>
<artifactId>bolt</artifactId>
Expand Down Expand Up @@ -553,6 +563,8 @@
<include>com.alipay.sofa:sofa-rpc-tracer-opentracing-resteasy</include>
<include>com.alipay.sofa:sofa-rpc-tracer-opentracing-triple</include>
<include>com.alipay.sofa:sofa-rpc-config-apollo</include>
<include>com.alipay.sofa:sofa-rpc-config-zk</include>
<include>com.alipay.sofa:sofa-rpc-config-nacos</include>
<include>com.alipay.sofa:sofa-rpc-doc-swagger</include>
<!-- TODO -->
</includes>
Expand Down
64 changes: 60 additions & 4 deletions ...p/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.alipay.sofa.rpc.client.Cluster;
import com.alipay.sofa.rpc.client.ClusterFactory;
import com.alipay.sofa.rpc.client.ProviderGroup;
import com.alipay.sofa.rpc.common.RpcConstants;
import com.alipay.sofa.rpc.common.SofaConfigs;
import com.alipay.sofa.rpc.common.SofaOptions;
import com.alipay.sofa.rpc.common.utils.CommonUtils;
Expand All @@ -28,6 +29,8 @@
import com.alipay.sofa.rpc.config.RegistryConfig;
import com.alipay.sofa.rpc.context.RpcRuntimeContext;
import com.alipay.sofa.rpc.core.exception.SofaRpcRuntimeException;
import com.alipay.sofa.rpc.dynamic.ConfigChangedEvent;
import com.alipay.sofa.rpc.dynamic.ConfigChangeType;
import com.alipay.sofa.rpc.dynamic.DynamicConfigKeys;
import com.alipay.sofa.rpc.dynamic.DynamicConfigManager;
import com.alipay.sofa.rpc.dynamic.DynamicConfigManagerFactory;
Expand All @@ -44,8 +47,10 @@

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CountDownLatch;
Expand All @@ -54,6 +59,7 @@
import java.util.concurrent.atomic.AtomicInteger;

import static com.alipay.sofa.rpc.common.RpcConstants.REGISTRY_PROTOCOL_DOMAIN;
import static com.alipay.sofa.common.config.SofaConfigs.getOrDefault;

/**
* Default consumer bootstrap.
Expand Down Expand Up @@ -146,7 +152,8 @@ public T refer() {
// build cluster
cluster = ClusterFactory.getCluster(this);
// build listeners
consumerConfig.setConfigListener(buildConfigListener(this));
ConfigListener configListener = buildConfigListener(this);
consumerConfig.setConfigListener(configListener);
consumerConfig.setProviderInfoListener(buildProviderInfoListener(this));
// init cluster
cluster.init();
Expand All @@ -156,13 +163,23 @@ public T refer() {
proxyIns = (T) ProxyFactory.buildProxy(consumerConfig.getProxy(), consumerConfig.getProxyClass(),
proxyInvoker);

//动态配置
//请求级别动态配置参数
final String dynamicAlias = consumerConfig.getParameter(DynamicConfigKeys.DYNAMIC_ALIAS);
if (StringUtils.isNotBlank(dynamicAlias)) {
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicAlias);
consumerConfig.getAppName(), dynamicAlias);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId());
}
Comment on lines +167 to 173
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for request-level dynamic configuration initialization.

The initialization of request-level dynamic configuration lacks error handling. If getDynamicManager returns null or initServiceConfiguration fails, it could lead to runtime errors.

Apply this diff to add error handling:

 if (StringUtils.isNotBlank(dynamicAlias)) {
+    try {
         final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
                 consumerConfig.getAppName(), dynamicAlias);
+        if (dynamicManager == null) {
+            LOGGER.warn("Failed to get DynamicConfigManager for alias: {}", dynamicAlias);
+            return proxyIns;
+        }
         dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId());
+    } catch (Exception e) {
+        LOGGER.error("Failed to initialize request-level dynamic configuration", e);
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//请求级别动态配置参数
final String dynamicAlias = consumerConfig.getParameter(DynamicConfigKeys.DYNAMIC_ALIAS);
if (StringUtils.isNotBlank(dynamicAlias)) {
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicAlias);
consumerConfig.getAppName(), dynamicAlias);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId());
}
//请求级别动态配置参数
final String dynamicAlias = consumerConfig.getParameter(DynamicConfigKeys.DYNAMIC_ALIAS);
if (StringUtils.isNotBlank(dynamicAlias)) {
try {
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicAlias);
if (dynamicManager == null) {
LOGGER.warn("Failed to get DynamicConfigManager for alias: {}", dynamicAlias);
return proxyIns;
}
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId());
} catch (Exception e) {
LOGGER.error("Failed to initialize request-level dynamic configuration", e);
}
}


//接口级别动态配置参数
final String dynamicUrl = getOrDefault(DynamicConfigKeys.CENTER_ADDRESS);
if ( StringUtils.isNotBlank(dynamicUrl)) {
//启用接口级别动态配置
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManagerWithUrl(
consumerConfig.getAppName(), dynamicUrl);
dynamicManager.addListener(consumerConfig.getInterfaceId(), configListener);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId(), configListener);
}
} catch (Exception e) {
if (cluster != null) {
cluster.destroy();
Expand Down Expand Up @@ -438,8 +455,47 @@ public void updateAllProviders(List<ProviderGroup> groups) {
*/
private class ConsumerAttributeListener implements ConfigListener {

private Map<String, String> newValueMap = new HashMap<>();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use ConcurrentHashMap for thread safety.

newValueMap is accessed from multiple methods that could be called concurrently. Using a regular HashMap is not thread-safe.

Apply this diff to use ConcurrentHashMap:

-private Map<String, String> newValueMap = new HashMap<>();
+private Map<String, String> newValueMap = new ConcurrentHashMap<>();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private Map<String, String> newValueMap = new HashMap<>();
private Map<String, String> newValueMap = new ConcurrentHashMap<>();

// 动态配置项
private final Set<String> dynamicConfigKeys = new HashSet<>();

ConsumerAttributeListener() {
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER);
}

@Override
public void process(ConfigChangedEvent event) {
// 清除上次的赋值，并保留赋值过的key
newValueMap.replaceAll((k, v) -> "");
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
// ADDED or MODIFIED
parseConfigurationLines(event.getContent());
}
attrUpdated(newValueMap);
}

private void parseConfigurationLines(String content) {
String[] lines = content.split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (dynamicConfigKeys.contains(tempKey)) {
newValueMap.put(key, value);
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure thread safety of newValueMap in ConsumerAttributeListener

The newValueMap is a HashMap that is accessed and modified in the process and parseConfigurationLines methods. If process can be called concurrently by multiple threads, this could lead to race conditions and inconsistent state. Consider synchronizing the process method or using a thread-safe map implementation, such as ConcurrentHashMap, for newValueMap.

Apply one of the following changes to fix the issue:

Option 1: Use ConcurrentHashMap for newValueMap:

- private Map<String, String> newValueMap = new HashMap<>();
+ private Map<String, String> newValueMap = new ConcurrentHashMap<>();

Option 2: Synchronize the process method:

- public void process(ConfigChangedEvent event) {
+ public synchronized void process(ConfigChangedEvent event) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private Map<String, String> newValueMap = new HashMap<>();
// 动态配置项
private final Set<String> dynamicConfigKeys = new HashSet<>();
ConsumerAttributeListener() {
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER);
}
@Override
public void process(ConfigChangedEvent event) {
// 清除上次的赋值，并保留赋值过的key
newValueMap.replaceAll((k, v) -> "");
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
// ADDED or MODIFIED
parseConfigurationLines(event.getContent());
}
attrUpdated(newValueMap);
}
private void parseConfigurationLines(String content) {
String[] lines = content.split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (dynamicConfigKeys.contains(tempKey)) {
newValueMap.put(key, value);
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}
private Map<String, String> newValueMap = new ConcurrentHashMap<>();
// 动态配置项
private final Set<String> dynamicConfigKeys = new HashSet<>();
ConsumerAttributeListener() {
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_TIMEOUT);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_RETRIES);
dynamicConfigKeys.add(RpcConstants.CONFIG_KEY_LOADBALANCER);
}
@Override
public void process(ConfigChangedEvent event) {
// 清除上次的赋值，并保留赋值过的key
newValueMap.replaceAll((k, v) -> "");
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
// ADDED or MODIFIED
parseConfigurationLines(event.getContent());
}
attrUpdated(newValueMap);
}
private void parseConfigurationLines(String content) {
String[] lines = content.split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (dynamicConfigKeys.contains(tempKey)) {
newValueMap.put(key, value);
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize configuration parsing for better performance.

The current implementation of parseConfigurationLines can be optimized by:

  1. Using a StringBuilder for string manipulations
  2. Pre-compiling the regex pattern for splitting
  3. Using a more efficient key extraction method

Apply this diff to optimize the method:

 private void parseConfigurationLines(String content) {
+    Pattern lineSplitter = Pattern.compile("\\n");
+    Pattern kvSplitter = Pattern.compile("=", 2);
-    String[] lines = content.split("\n");
+    String[] lines = lineSplitter.split(content);
     for (String line : lines) {
-        String[] keyValue = line.split("=", 2);
+        String[] keyValue = kvSplitter.split(line, 2);
         if (keyValue.length == 2) {
             String key = keyValue[0].trim();
             String value = keyValue[1].trim();
-            String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
+            int lastDotIndex = key.lastIndexOf('.');
+            String tempKey = lastDotIndex == -1 ? key : key.substring(lastDotIndex + 1);
             if (dynamicConfigKeys.contains(tempKey)) {
                 newValueMap.put(key, value);
             }
         } else {
             LOGGER.warn("Malformed configuration line: {}", line);
         }
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private void parseConfigurationLines(String content) {
String[] lines = content.split("\n");
for (String line : lines) {
String[] keyValue = line.split("=", 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (dynamicConfigKeys.contains(tempKey)) {
newValueMap.put(key, value);
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}
private void parseConfigurationLines(String content) {
Pattern lineSplitter = Pattern.compile("\\n");
Pattern kvSplitter = Pattern.compile("=", 2);
String[] lines = lineSplitter.split(content);
for (String line : lines) {
String[] keyValue = kvSplitter.split(line, 2);
if (keyValue.length == 2) {
String key = keyValue[0].trim();
String value = keyValue[1].trim();
int lastDotIndex = key.lastIndexOf('.');
String tempKey = lastDotIndex == -1 ? key : key.substring(lastDotIndex + 1);
if (dynamicConfigKeys.contains(tempKey)) {
newValueMap.put(key, value);
}
} else {
LOGGER.warn("Malformed configuration line: {}", line);
}
}
}


@Override
public void configChanged(Map newValue) {
public void configChanged(Map newValueMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused configChanged method with empty implementation

The method public void configChanged(Map newValueMap) is declared but has an empty body:

public void configChanged(Map newValueMap) {

}

Since it doesn't perform any actions and isn't called within this class, it may be unnecessary.

If this method is not required, consider removing it to clean up the code. If it's intended for future use or to fulfill an interface contract, consider adding a comment explaining its purpose.


}

Expand Down
104 changes: 95 additions & 9 deletions ...g-apollo/src/main/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,24 @@
*/
package com.alipay.sofa.rpc.dynamic.apollo;

import com.alipay.sofa.common.config.SofaConfigs;
import com.alipay.sofa.rpc.auth.AuthRuleGroup;
import com.alipay.sofa.rpc.dynamic.DynamicConfigKeyHelper;
import com.alipay.sofa.rpc.dynamic.DynamicConfigManager;
import com.alipay.sofa.rpc.dynamic.DynamicHelper;
import com.alipay.sofa.rpc.common.utils.StringUtils;
import com.alipay.sofa.rpc.dynamic.*;
import com.alipay.sofa.rpc.ext.Extension;
import com.alipay.sofa.rpc.listener.ConfigListener;
import com.ctrip.framework.apollo.Config;
import com.ctrip.framework.apollo.ConfigChangeListener;
import com.ctrip.framework.apollo.ConfigService;
import com.ctrip.framework.apollo.enums.PropertyChangeType;
import com.ctrip.framework.apollo.model.ConfigChange;

import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CopyOnWriteArraySet;

/**
* @author bystander
Expand All @@ -34,41 +45,77 @@
@Extension(value = "apollo", override = true)
public class ApolloDynamicConfigManager extends DynamicConfigManager {

private static final String APOLLO_APPID_KEY = "app.id";

private static final String APOLLO_ADDR_KEY = "apollo.meta";

private static final String APOLLO_CLUSTER_KEY = "apollo.cluster";

private static final String APOLLO_PROTOCOL_PREFIX = "http://";

private Config config;

private final ConcurrentMap<String, ApolloListener> watchListenerMap = new ConcurrentHashMap<>();

protected ApolloDynamicConfigManager(String appName) {
super(appName);
super(appName, SofaConfigs.getOrCustomDefault(DynamicConfigKeys.APOLLO_ADDRESS,""));
if (StringUtils.isNotBlank(appName)) {
System.setProperty(APOLLO_APPID_KEY, appName);
}
if (StringUtils.isNotBlank(getAddress())) {
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
}
config = ConfigService.getAppConfig();
}

protected ApolloDynamicConfigManager(String appName, String remainUrl) {
super(appName, remainUrl);
System.setProperty(APOLLO_APPID_KEY, appName);
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add a check for empty address before setting apollo.meta

In the constructor, getAddress() may return an empty string. Appending it to APOLLO_PROTOCOL_PREFIX results in apollo.meta being set to "http://", which is likely invalid and could lead to misconfigurations.

Consider adding a check to ensure getAddress() is not blank before setting the system property, similar to the check in the other constructor.

Apply this diff to add the check:

protected ApolloDynamicConfigManager(String appName, String remainUrl) {
    super(appName, remainUrl);
    System.setProperty(APOLLO_APPID_KEY, appName);
+   if (StringUtils.isNotBlank(getAddress())) {
        System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
+   }
    Map params = getParams();
    if (params != null && params.containsKey("cluster")) {
        String clusterValue = (String) params.get("cluster");
        System.setProperty(APOLLO_CLUSTER_KEY, clusterValue);
    }
    config = ConfigService.getAppConfig();
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
protected ApolloDynamicConfigManager(String appName, String remainUrl) {
super(appName, remainUrl);
System.setProperty(APOLLO_APPID_KEY, appName);
if (StringUtils.isNotBlank(getAddress())) {
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getAddress());
}
Map params = getParams();
if (params != null && params.containsKey("cluster")) {
String clusterValue = (String) params.get("cluster");
System.setProperty(APOLLO_CLUSTER_KEY, clusterValue);
}
config = ConfigService.getAppConfig();
}

Map params = getParams();
if (params != null && params.containsKey("cluster")) {
String clusterValue = (String)params.get("cluster");
System.setProperty(APOLLO_CLUSTER_KEY, clusterValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Specify Generic Types for Map Usage

The params variable is declared without generic types, which can lead to unchecked type warnings and potential ClassCastExceptions. It's good practice to specify the generic types for collections to ensure type safety.

Apply this diff to specify the generic types:

- Map params = getParams();
+ Map<String, Object> params = getParams();

Committable suggestion was skipped due to low confidence.

}
config = ConfigService.getAppConfig();
}

@Override
public void initServiceConfiguration(String service) {
//TODO not now
// TODO 暂不支持
}
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement initServiceConfiguration(String service) Method

The method initServiceConfiguration(String service) contains a TODO comment indicating it's currently not supported. Leaving it unimplemented might lead to AbstractMethodError if called. Consider implementing the method or throwing an UnsupportedOperationException to handle this scenario gracefully.

Would you like assistance in implementing this method or should we update it to throw an exception? I can help draft the implementation or open a GitHub issue to track this task.

Example of throwing an exception:

@Override
public void initServiceConfiguration(String service) {
-    // TODO 暂不支持
+    throw new UnsupportedOperationException("initServiceConfiguration is not supported yet.");
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// TODO 暂不支持
}
@Override
public void initServiceConfiguration(String service) {
throw new UnsupportedOperationException("initServiceConfiguration is not supported yet.");
}


@Override
public void initServiceConfiguration(String service, ConfigListener listener) {
String rawConfig = config.getProperty(service, "");
if (StringUtils.isNotBlank(rawConfig)) {
listener.process(new ConfigChangedEvent(service, rawConfig));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Notify listener even when configuration is blank

In the initServiceConfiguration method, the listener is only notified if rawConfig is not blank. This could lead to the listener missing updates or not being initialized properly when the configuration is empty. It's better to notify the listener regardless of the configuration content to ensure correct behavior.

Apply this diff to notify the listener in all cases:

if (StringUtils.isNotBlank(rawConfig)) {
    listener.process(new ConfigChangedEvent(service, rawConfig));
+} else {
+    listener.process(new ConfigChangedEvent(service, rawConfig));
}

Or simply remove the condition:

-if (StringUtils.isNotBlank(rawConfig)) {
    listener.process(new ConfigChangedEvent(service, rawConfig));
-}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
String rawConfig = config.getProperty(service, "");
if (StringUtils.isNotBlank(rawConfig)) {
listener.process(new ConfigChangedEvent(service, rawConfig));
}
String rawConfig = config.getProperty(service, "");
listener.process(new ConfigChangedEvent(service, rawConfig));

}

@Override
public String getProviderServiceProperty(String service, String key) {
return config.getProperty(DynamicConfigKeyHelper.buildProviderServiceProKey(service, key),
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
}

@Override
public String getConsumerServiceProperty(String service, String key) {
return config.getProperty(DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key),
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
DynamicHelper.DEFAULT_DYNAMIC_VALUE);

}

@Override
public String getProviderMethodProperty(String service, String method, String key) {
return config.getProperty(DynamicConfigKeyHelper.buildProviderMethodProKey(service, method, key),
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
}

@Override
public String getConsumerMethodProperty(String service, String method, String key) {
return config.getProperty(DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key),
DynamicHelper.DEFAULT_DYNAMIC_VALUE);
DynamicHelper.DEFAULT_DYNAMIC_VALUE);

Comment on lines 115 to 136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for Apollo configuration retrieval

The service property getter methods don't handle potential exceptions from Apollo's getProperty calls. This could lead to uncaught exceptions in production.

Add try-catch blocks to handle Apollo exceptions gracefully:

 @Override
 public String getProviderServiceProperty(String service, String key) {
+    try {
         return config.getProperty(DynamicConfigKeyHelper.buildProviderServiceProKey(service, key),
                 DynamicHelper.DEFAULT_DYNAMIC_VALUE);
+    } catch (Exception e) {
+        LOGGER.error("Failed to get provider service property for service: " + service + ", key: " + key, e);
+        return DynamicHelper.DEFAULT_DYNAMIC_VALUE;
+    }
 }

Committable suggestion was skipped due to low confidence.

}

Expand All @@ -77,4 +124,43 @@ public AuthRuleGroup getServiceAuthRule(String service) {
//TODO 暂不支持
return null;
}

@Override
public void addListener(String key, ConfigListener listener) {
ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener());
apolloListener.addListener(listener);
config.addChangeListener(apolloListener, Collections.singleton(key));
}
Comment on lines +146 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance listener management to prevent memory leaks

The current implementation lacks listener cleanup capabilities and doesn't prevent duplicate registrations.

Consider these improvements:

@Override
public void addListener(String key, ConfigListener listener) {
    ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener());
+    if (apolloListener.hasListener(listener)) {
+        return;  // Prevent duplicate registration
+    }
    apolloListener.addListener(listener);
    config.addChangeListener(apolloListener, Collections.singleton(key));
}

+@Override
+public void removeListener(String key, ConfigListener listener) {
+    ApolloListener apolloListener = watchListenerMap.get(key);
+    if (apolloListener != null) {
+        apolloListener.removeListener(listener);
+        if (apolloListener.isEmpty()) {
+            config.removeChangeListener(apolloListener);
+            watchListenerMap.remove(key);
+        }
+    }
}

Also add these methods to the ApolloListener class:

boolean hasListener(ConfigListener listener) {
    return listeners.contains(listener);
}

boolean isEmpty() {
    return listeners.isEmpty();
}

void removeListener(ConfigListener listener) {
    listeners.remove(listener);
}


public class ApolloListener implements ConfigChangeListener {

private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();

ApolloListener() {
}

@Override
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
for (String key : changeEvent.changedKeys()) {
ConfigChange change = changeEvent.getChange(key);
ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
listeners.forEach(listener -> listener.process(event));
}
}
Comment on lines +157 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent Concurrent Modification Exceptions

Modifying the listeners set while iterating over it in the onChange method could lead to ConcurrentModificationException. Although CopyOnWriteArraySet is thread-safe for concurrent reads and writes, it's safer to iterate over a snapshot of the set.

Consider iterating over a snapshot of the listeners:

public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
    for (String key : changeEvent.changedKeys()) {
        ConfigChange change = changeEvent.getChange(key);
        ConfigChangedEvent event =
                new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
-       listeners.forEach(listener -> listener.process(event));
+       new ArrayList<>(listeners).forEach(listener -> listener.process(event));
    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
for (String key : changeEvent.changedKeys()) {
ConfigChange change = changeEvent.getChange(key);
ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
listeners.forEach(listener -> listener.process(event));
}
}
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
for (String key : changeEvent.changedKeys()) {
ConfigChange change = changeEvent.getChange(key);
ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change));
new ArrayList<>(listeners).forEach(listener -> listener.process(event));
}
}


private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}
Comment on lines +166 to +174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle All Possible Change Types Explicitly

In the getChangeType method, all unhandled PropertyChangeTypes default to MODIFIED. If new change types are introduced in the future, this could lead to unexpected behavior.

Consider modifying the method to explicitly handle all possible change types or throw an exception for unknown types:

private ConfigChangeType getChangeType(ConfigChange change) {
    if (change.getChangeType() == PropertyChangeType.DELETED) {
        return ConfigChangeType.DELETED;
    }
    if (change.getChangeType() == PropertyChangeType.ADDED) {
        return ConfigChangeType.ADDED;
    }
+   if (change.getChangeType() == PropertyChangeType.MODIFIED) {
+       return ConfigChangeType.MODIFIED;
+   }
+   throw new IllegalArgumentException("Unknown change type: " + change.getChangeType());
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
if (change.getChangeType() == PropertyChangeType.MODIFIED) {
return ConfigChangeType.MODIFIED;
}
throw new IllegalArgumentException("Unknown change type: " + change.getChangeType());
}


void addListener(ConfigListener configListener) {
this.listeners.add(configListener);
}
Comment on lines +176 to +178
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Provide a method to remove listeners

The ApolloListener class currently allows adding listeners but does not provide a way to remove them. Over time, this can lead to memory leaks or unnecessary processing if listeners are no longer needed. Consider adding a removeListener method to allow for proper listener management.

Implement the removeListener method as follows:

void addListener(ConfigListener configListener) {
    this.listeners.add(configListener);
}

+void removeListener(ConfigListener configListener) {
+    this.listeners.remove(configListener);
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void addListener(ConfigListener configListener) {
this.listeners.add(configListener);
}
void addListener(ConfigListener configListener) {
this.listeners.add(configListener);
}
void removeListener(ConfigListener configListener) {
this.listeners.remove(configListener);
}

}
}
19 changes: 12 additions & 7 deletions ...ollo/src/test/java/com/alipay/sofa/rpc/dynamic/apollo/ApolloDynamicConfigManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package com.alipay.sofa.rpc.dynamic.apollo;

import com.alipay.sofa.rpc.dynamic.DynamicConfigManager;
import com.alipay.sofa.rpc.dynamic.DynamicConfigManagerFactory;
import com.alipay.sofa.rpc.dynamic.DynamicHelper;
import com.alipay.sofa.rpc.log.Logger;
import com.alipay.sofa.rpc.log.LoggerFactory;
Expand All @@ -24,10 +26,11 @@

public class ApolloDynamicConfigManagerTest {

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

private ApolloDynamicConfigManager apolloDynamicConfigManager = new ApolloDynamicConfigManager("test");
private DynamicConfigManager apolloDynamicConfigManager = DynamicConfigManagerFactory.getDynamicManager("test",
"apollo");

@Test
public void getProviderServiceProperty() {
Expand All @@ -37,17 +40,19 @@ public void getProviderServiceProperty() {

@Test
public void getConsumerServiceProperty() {
String result = apolloDynamicConfigManager.getConsumerServiceProperty("serviceName", "timeout");
Assert.assertEquals(DynamicHelper.DEFAULT_DYNAMIC_VALUE, result);
}

@Test
public void getProviderMethodProperty() {
String result = apolloDynamicConfigManager.getProviderMethodProperty("serviceName", "methodName", "timeout");
Assert.assertEquals(DynamicHelper.DEFAULT_DYNAMIC_VALUE, result);
}

@Test
public void getConsumerMethodProperty() {
}

@Test
public void getServiceAuthRule() {
String result = apolloDynamicConfigManager.getConsumerMethodProperty("serviceName", "methodName", "timeout");
Assert.assertEquals(DynamicHelper.DEFAULT_DYNAMIC_VALUE, result);
Comment on lines +43 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage with additional test scenarios.

The current tests only verify default values. Consider adding:

  1. Tests with actual configuration values
  2. Negative test cases (null/invalid service names, methods)
  3. Tests verifying dynamic config changes
  4. Tests for initialization failures

Example test to add:

@Test
public void testDynamicConfigUpdate() {
    // Setup initial value
    String serviceName = "testService";
    String property = "timeout";
    String initialValue = apolloDynamicConfigManager.getConsumerServiceProperty(serviceName, property);
    
    // Simulate config change
    // TODO: Add mechanism to trigger config change
    
    // Verify updated value
    String updatedValue = apolloDynamicConfigManager.getConsumerServiceProperty(serviceName, property);
    Assert.assertNotEquals("Config should be updated", initialValue, updatedValue);
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidServiceName() {
    apolloDynamicConfigManager.getConsumerServiceProperty(null, "timeout");
}

}
}
Loading
Loading