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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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
5 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
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());
}

//接口级别动态配置参数
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<>();

// 动态配置项
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);
}
}
}
Comment on lines +458 to +495
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);
}
}
}


@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);
Comment on lines +76 to +78
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 +85 to +86
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));
}
}

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

}

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 +129 to +133
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 Listeners Are Not Added Multiple Times

In the addListener method, adding the same ApolloListener to config multiple times for the same key could result in duplicate notifications. Ensure that the listener is registered only once per key to prevent redundant processing.

Consider checking if the listener has already been added before registering it:

public void addListener(String key, ConfigListener listener) {
    ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener());
    apolloListener.addListener(listener);
+   if (!apolloListener.isRegistered()) {
        config.addChangeListener(apolloListener, Collections.singleton(key));
+       apolloListener.setRegistered(true);
+   }
}

You'll need to add isRegistered and setRegistered methods to the ApolloListener class.

Committable suggestion was skipped due to low confidence.


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 +143 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.

⚠️ 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 +152 to +160
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);
}
}
}
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);
}
}
Loading
Loading