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 all 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
69 changes: 64 additions & 5 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,9 +29,12 @@
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;
import com.alipay.sofa.rpc.dynamic.DynamicUrl;
import com.alipay.sofa.rpc.ext.Extension;
import com.alipay.sofa.rpc.invoke.Invoker;
import com.alipay.sofa.rpc.listener.ConfigListener;
Expand All @@ -44,8 +48,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 +60,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 +153,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 +164,25 @@ 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);
}
}


//接口级别动态配置参数
Boolean dynamicConfigRefreshEnable = getOrDefault(DynamicConfigKeys.DYNAMIC_REFRESH_ENABLE);
String configCenterAddress = getOrDefault(DynamicConfigKeys.CONFIG_CENTER_ADDRESS);
if (dynamicConfigRefreshEnable && StringUtils.isNotBlank(configCenterAddress)) {
DynamicUrl dynamicUrl = new DynamicUrl(configCenterAddress);
//启用接口级别动态配置
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicUrl.getProtocol());
dynamicManager.addListener(consumerConfig.getInterfaceId(), configListener);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId(), configListener);
}
Comment on lines +175 to +185
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 cleanup logic for interface-level dynamic configuration.

The interface-level configuration initialization lacks cleanup logic. If initialization fails after adding the listener, the listener remains registered, potentially causing memory leaks.

Apply this diff:

 if (dynamicConfigRefreshEnable && StringUtils.isNotBlank(configCenterAddress)) {
+    DynamicConfigManager dynamicManager = null;
+    ConfigListener registeredListener = null;
     try {
         DynamicUrl dynamicUrl = new DynamicUrl(configCenterAddress);
-        final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
+        dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
                 consumerConfig.getAppName(), dynamicUrl.getProtocol());
+        if (dynamicManager == null) {
+            LOGGER.warn("Failed to get DynamicConfigManager for protocol: {}", dynamicUrl.getProtocol());
+            return proxyIns;
+        }
         dynamicManager.addListener(consumerConfig.getInterfaceId(), configListener);
+        registeredListener = configListener;
         dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId(), configListener);
+    } catch (Exception e) {
+        LOGGER.error("Failed to initialize interface-level dynamic configuration", e);
+        if (dynamicManager != null && registeredListener != null) {
+            dynamicManager.removeListener(consumerConfig.getInterfaceId(), registeredListener);
+        }
     }
 }
📝 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
//接口级别动态配置参数
Boolean dynamicConfigRefreshEnable = getOrDefault(DynamicConfigKeys.DYNAMIC_REFRESH_ENABLE);
String configCenterAddress = getOrDefault(DynamicConfigKeys.CONFIG_CENTER_ADDRESS);
if (dynamicConfigRefreshEnable && StringUtils.isNotBlank(configCenterAddress)) {
DynamicUrl dynamicUrl = new DynamicUrl(configCenterAddress);
//启用接口级别动态配置
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicUrl.getProtocol());
dynamicManager.addListener(consumerConfig.getInterfaceId(), configListener);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId(), configListener);
}
//接口级别动态配置参数
Boolean dynamicConfigRefreshEnable = getOrDefault(DynamicConfigKeys.DYNAMIC_REFRESH_ENABLE);
String configCenterAddress = getOrDefault(DynamicConfigKeys.CONFIG_CENTER_ADDRESS);
if (dynamicConfigRefreshEnable && StringUtils.isNotBlank(configCenterAddress)) {
DynamicConfigManager dynamicManager = null;
ConfigListener registeredListener = null;
try {
DynamicUrl dynamicUrl = new DynamicUrl(configCenterAddress);
dynamicManager = DynamicConfigManagerFactory.getDynamicManager(
consumerConfig.getAppName(), dynamicUrl.getProtocol());
if (dynamicManager == null) {
LOGGER.warn("Failed to get DynamicConfigManager for protocol: {}", dynamicUrl.getProtocol());
return proxyIns;
}
dynamicManager.addListener(consumerConfig.getInterfaceId(), configListener);
registeredListener = configListener;
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId(), configListener);
} catch (Exception e) {
LOGGER.error("Failed to initialize interface-level dynamic configuration", e);
if (dynamicManager != null && registeredListener != null) {
dynamicManager.removeListener(consumerConfig.getInterfaceId(), registeredListener);
}
}
}

} catch (Exception e) {
if (cluster != null) {
cluster.destroy();
Expand Down Expand Up @@ -438,8 +458,47 @@ public void updateAllProviders(List<ProviderGroup> groups) {
*/
private class ConsumerAttributeListener implements ConfigListener {

// 可以动态配置的选项
private final Set<String> supportDynamicConfigKeys = new HashSet<>();
private final Map<String, String> newValueMap = new HashMap<>();

Comment on lines +461 to +464
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.

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

Apply this diff:

-private final Map<String, String> newValueMap = new HashMap<>();
+private final 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 final Set<String> supportDynamicConfigKeys = new HashSet<>();
private final Map<String, String> newValueMap = new HashMap<>();
// 可以动态配置的选项
private final Set<String> supportDynamicConfigKeys = new HashSet<>();
private final Map<String, String> newValueMap = new ConcurrentHashMap<>();

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

@Override
public void process(ConfigChangedEvent event) {
// 清除上次的动态配置值缓存
consumerConfig.getDynamicConfigValueCache().clear();
// 获取对应配置项的默认值
for (String key : newValueMap.keySet()) {
if (consumerConfig.getConfigValueCache().get(key) != null) {
newValueMap.put(key, String.valueOf(consumerConfig.getConfigValueCache().get(key)));
} else {
newValueMap.put(key, null);
}
}
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
// ADDED or MODIFIED
Map<String, String> dynamicValueMap = event.getDynamicValueMap();
for (String key : dynamicValueMap.keySet()) {
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (supportDynamicConfigKeys.contains(tempKey)) {
String value = dynamicValueMap.get(key);
if (StringUtils.isNotBlank(value)) {
consumerConfig.getDynamicConfigValueCache().put(key, value);
newValueMap.put(key, value);
}
}
}
}
attrUpdated(newValueMap);
}
Comment on lines +471 to +498
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 configuration parsing in the process method can be optimized by:

  1. Using StringBuilder for string manipulations
  2. Avoiding repeated substring operations
  3. Using more efficient key extraction

Apply this diff:

 @Override
 public void process(ConfigChangedEvent event) {
     consumerConfig.getDynamicConfigValueCache().clear();
-    for (String key : newValueMap.keySet()) {
-        if (consumerConfig.getConfigValueCache().get(key) != null) {
-            newValueMap.put(key, String.valueOf(consumerConfig.getConfigValueCache().get(key)));
-        } else {
-            newValueMap.put(key, null);
-        }
-    }
+    // Batch update default values
+    newValueMap.replaceAll((key, value) -> {
+        Object defaultValue = consumerConfig.getConfigValueCache().get(key);
+        return defaultValue != null ? String.valueOf(defaultValue) : null;
+    });

     if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
         Map<String, String> dynamicValueMap = event.getDynamicValueMap();
         for (String key : dynamicValueMap.keySet()) {
-            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 (supportDynamicConfigKeys.contains(tempKey)) {
                 String value = dynamicValueMap.get(key);
                 if (StringUtils.isNotBlank(value)) {
-                    consumerConfig.getDynamicConfigValueCache().put(key, value);
-                    newValueMap.put(key, value);
+                    // Batch update both caches
+                    consumerConfig.getDynamicConfigValueCache().put(key, value);
+                    newValueMap.put(key, value);
                 }
             }
         }
     }
     attrUpdated(newValueMap);
 }
📝 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
@Override
public void process(ConfigChangedEvent event) {
// 清除上次的动态配置值缓存
consumerConfig.getDynamicConfigValueCache().clear();
// 获取对应配置项的默认值
for (String key : newValueMap.keySet()) {
if (consumerConfig.getConfigValueCache().get(key) != null) {
newValueMap.put(key, String.valueOf(consumerConfig.getConfigValueCache().get(key)));
} else {
newValueMap.put(key, null);
}
}
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
// ADDED or MODIFIED
Map<String, String> dynamicValueMap = event.getDynamicValueMap();
for (String key : dynamicValueMap.keySet()) {
String tempKey = key.lastIndexOf(".") == -1 ? key : key.substring(key.lastIndexOf(".") + 1);
if (supportDynamicConfigKeys.contains(tempKey)) {
String value = dynamicValueMap.get(key);
if (StringUtils.isNotBlank(value)) {
consumerConfig.getDynamicConfigValueCache().put(key, value);
newValueMap.put(key, value);
}
}
}
}
attrUpdated(newValueMap);
}
@Override
public void process(ConfigChangedEvent event) {
consumerConfig.getDynamicConfigValueCache().clear();
// Batch update default values
newValueMap.replaceAll((key, value) -> {
Object defaultValue = consumerConfig.getConfigValueCache().get(key);
return defaultValue != null ? String.valueOf(defaultValue) : null;
});
if (!event.getChangeType().equals(ConfigChangeType.DELETED)) {
Map<String, String> dynamicValueMap = event.getDynamicValueMap();
for (String key : dynamicValueMap.keySet()) {
int lastDotIndex = key.lastIndexOf('.');
String tempKey = lastDotIndex == -1 ? key : key.substring(lastDotIndex + 1);
if (supportDynamicConfigKeys.contains(tempKey)) {
String value = dynamicValueMap.get(key);
if (StringUtils.isNotBlank(value)) {
// Batch update both caches
consumerConfig.getDynamicConfigValueCache().put(key, value);
newValueMap.put(key, value);
}
}
}
}
attrUpdated(newValueMap);
}


@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 All @@ -452,7 +511,7 @@ public synchronized void attrUpdated(Map newValueMap) {
Map<String, String> oldValues = new HashMap<String, String>();
boolean rerefer = false;
try { // 检查是否有变化
// 是否过滤map?
// 是否过滤map?
for (Map.Entry<String, String> entry : newValues.entrySet()) {
String newValue = entry.getValue();
String oldValue = consumerConfig.queryAttribute(entry.getKey());
Expand Down
116 changes: 108 additions & 8 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,30 @@
*/
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.common.utils.StringUtils;
import com.alipay.sofa.rpc.dynamic.ConfigChangeType;
import com.alipay.sofa.rpc.dynamic.ConfigChangedEvent;
import com.alipay.sofa.rpc.dynamic.DynamicConfigKeyHelper;
import com.alipay.sofa.rpc.dynamic.DynamicConfigKeys;
import com.alipay.sofa.rpc.dynamic.DynamicConfigManager;
import com.alipay.sofa.rpc.dynamic.DynamicHelper;
import com.alipay.sofa.rpc.ext.Extension;
import com.alipay.sofa.rpc.listener.ConfigListener;
import com.alipay.sofa.rpc.log.Logger;
import com.alipay.sofa.rpc.log.LoggerFactory;
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.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CopyOnWriteArraySet;

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

private Config config;
private final static Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);

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_PARAM_APPID_KEY = "appId";

private static final String APOLLO_PARAM_CLUSTER_KEY = "cluster";

private static final String APOLLO_PARAM_NAMESPACE_KEY = "namespace";

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

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

private final Config config;

protected ApolloDynamicConfigManager(String appName) {
super(appName);
config = ConfigService.getAppConfig();
super(appName, SofaConfigs.getOrCustomDefault(DynamicConfigKeys.CONFIG_CENTER_ADDRESS, ""));
if (getDynamicUrl() != null) {
if (StringUtils.isNotBlank(getDynamicUrl().getParam(APOLLO_PARAM_APPID_KEY))) {
System.setProperty(APOLLO_APPID_KEY, getDynamicUrl().getParam(APOLLO_PARAM_APPID_KEY));
}
if (StringUtils.isNotBlank(getDynamicUrl().getAddress())) {
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getDynamicUrl().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

Avoid hardcoding protocol when setting apollo.meta

Currently, the code appends APOLLO_PROTOCOL_PREFIX ("http://") to the address when setting the apollo.meta system property. This might cause issues if the address already includes the protocol or if a different protocol (like HTTPS) is needed. Consider using the address as-is or making the protocol prefix configurable.

Apply this diff to use the address directly:

if (StringUtils.isNotBlank(getDynamicUrl().getAddress())) {
-    System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + getDynamicUrl().getAddress());
+    System.setProperty(APOLLO_ADDR_KEY, getDynamicUrl().getAddress());
}

Committable suggestion was skipped due to low confidence.

}
if (StringUtils.isNotBlank(getDynamicUrl().getParam(APOLLO_PARAM_CLUSTER_KEY))) {
System.setProperty(APOLLO_CLUSTER_KEY, getDynamicUrl().getParam(APOLLO_PARAM_CLUSTER_KEY));
}
if (StringUtils.isNotBlank(getDynamicUrl().getParam(APOLLO_PARAM_NAMESPACE_KEY))) {
config = ConfigService.getConfig(getDynamicUrl().getParam(APOLLO_PARAM_NAMESPACE_KEY));
} else {
config = ConfigService.getAppConfig();
}
} else {
config = ConfigService.getAppConfig();
}
Comment on lines 74 to +93
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

Simplify constructor logic and add null checks

The constructor contains complex initialization logic and potential null pointer risks. Consider:

  1. Extract the Apollo configuration initialization into a separate method
  2. Add null checks before accessing getDynamicUrl() parameters

Apply this diff to improve the code:

 protected ApolloDynamicConfigManager(String appName) {
     super(appName, SofaConfigs.getOrCustomDefault(DynamicConfigKeys.CONFIG_CENTER_ADDRESS, ""));
+    initApolloConfig();
+ }
+
+ private void initApolloConfig() {
     if (getDynamicUrl() != null) {
-        if (StringUtils.isNotBlank(getDynamicUrl().getParam(APOLLO_PARAM_APPID_KEY))) {
+        String appId = getDynamicUrl().getParam(APOLLO_PARAM_APPID_KEY);
+        if (StringUtils.isNotBlank(appId)) {
-            System.setProperty(APOLLO_APPID_KEY, getDynamicUrl().getParam(APOLLO_PARAM_APPID_KEY));
+            System.setProperty(APOLLO_APPID_KEY, appId);
         }
         // Similar changes for other parameters...
     }
     config = ConfigService.getAppConfig();
 }

Committable suggestion was skipped due to low confidence.

}

@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) {
try {
String rawConfig = config.getProperty(service, "");
if (StringUtils.isNotBlank(rawConfig)) {
listener.process(new ConfigChangedEvent(service, rawConfig));
}
} catch (Exception e) {
LOGGER.error("Init service configuration error", e);
}
}

@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 +141,40 @@ 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 static class ApolloListener implements ConfigChangeListener {

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

@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);
}

Comment on lines +152 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

Add listener management capabilities to ApolloListener

The ApolloListener class lacks methods for checking listener existence and removing listeners. This could lead to memory leaks and duplicate notifications.

Add these methods to improve listener management:

 public static class ApolloListener implements ConfigChangeListener {
     private final Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();
+    private boolean isRegistered = false;
+
+    boolean isRegistered() {
+        return isRegistered;
+    }
+
+    void setRegistered(boolean registered) {
+        isRegistered = registered;
+    }
+
+    boolean hasListener(ConfigListener listener) {
+        return listeners.contains(listener);
+    }
+
+    void removeListener(ConfigListener listener) {
+        listeners.remove(listener);
+    }
 }
📝 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 static class ApolloListener implements ConfigChangeListener {
private final Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();
@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));
}
}
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}
void addListener(ConfigListener configListener) {
this.listeners.add(configListener);
}
public static class ApolloListener implements ConfigChangeListener {
private final Set<ConfigListener> listeners = new CopyOnWriteArraySet<>();
private boolean isRegistered = false;
boolean isRegistered() {
return isRegistered;
}
void setRegistered(boolean registered) {
isRegistered = registered;
}
boolean hasListener(ConfigListener listener) {
return listeners.contains(listener);
}
void removeListener(ConfigListener listener) {
listeners.remove(listener);
}
@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));
}
}
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}
void addListener(ConfigListener configListener) {
this.listeners.add(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