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 2 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 @@ -310,6 +310,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 @@ -565,6 +575,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
73 changes: 67 additions & 6 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 @@ -28,6 +28,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 @@ -42,10 +44,7 @@
import com.alipay.sofa.rpc.registry.Registry;
import com.alipay.sofa.rpc.registry.RegistryFactory;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
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

Avoid using wildcard imports; import specific classes instead

Using wildcard imports like import java.util.*; can lead to namespace pollution and may introduce ambiguity between class names. It's best practice to import only the specific classes you need.

Apply this diff to import specific classes:

-import java.util.*;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;

Committable suggestion was skipped due to low confidence.

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CountDownLatch;
Expand Down Expand Up @@ -156,13 +155,26 @@ 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);
dynamicManager.initServiceConfiguration(consumerConfig.getInterfaceId());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ConfigListener for request-level dynamic configuration

In the request-level dynamic configuration setup, you initialize the DynamicConfigManager and call initServiceConfiguration. However, unlike the interface-level configuration, you do not set a ConfigListener. This could prevent the consumer from responding to configuration changes at the request level.

Consider adding a ConfigListener for request-level dynamic configuration to ensure that configuration changes are properly handled.

//启用接口级别动态配置
final String dynamicUrl = consumerConfig.getParameter(DynamicConfigKeys.DYNAMIC_URL);
if (StringUtils.isNotBlank(dynamicUrl)) {
final DynamicConfigManager dynamicManager = DynamicConfigManagerFactory.getDynamicManagerWithUrl(
consumerConfig.getAppName(), dynamicUrl);

//build listeners for dynamic config
consumerConfig.setConfigListener(buildConfigListener(this,dynamicManager));
}



Copy link
Contributor

Choose a reason for hiding this comment

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

Overwriting existing ConfigListener may lead to unintended behavior

In the interface-level dynamic configuration, you set a new ConfigListener on the consumerConfig using:

consumerConfig.setConfigListener(buildConfigListener(this, dynamicManager));

Since consumerConfig may already have a ConfigListener set earlier in the code, this operation could overwrite the existing listener, potentially disrupting previously established configurations or listeners.

Consider modifying the implementation to support multiple ConfigListeners or ensure that you're not unintentionally overwriting an existing listener. You might implement a mechanism to aggregate listeners or check if a listener is already set before assigning a new one.

} catch (Exception e) {
if (cluster != null) {
cluster.destroy();
Expand Down Expand Up @@ -202,6 +214,17 @@ protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap) {
return new ConsumerAttributeListener();
}

/**
* Build ConfigListener for consumer bootstrap with dynamic config.
*
* @param bootstrap ConsumerBootstrap
* @param dynamicManager DynamicConfigManager
* @return ConfigListener
*/
protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap, DynamicConfigManager dynamicManager) {
return new ConsumerAttributeListener(dynamicManager);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the usage of overloaded buildConfigListener methods

The introduction of an overloaded buildConfigListener method adds flexibility, but it may cause confusion due to the similar method names and parameters:

  • protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap)
  • protected ConfigListener buildConfigListener(ConsumerBootstrap bootstrap, DynamicConfigManager dynamicManager)

To improve code readability and maintainability, consider renaming one of the methods to more clearly reflect its purpose, such as buildDynamicConfigListener. This differentiation helps future developers understand the distinction without examining method signatures.

/**
* Build ProviderInfoListener for consumer bootstrap.
*
Expand Down Expand Up @@ -438,8 +461,46 @@ 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<>();

ConsumerAttributeListener() {

}

ConsumerAttributeListener(DynamicConfigManager dynamicManager) {
this.initWith(consumerConfig.getInterfaceId(),dynamicManager);
}

public void initWith(String key, DynamicConfigManager dynamicManager) {
dynamicManager.addListener(key, this);
// 初始化配置值
String rawConfig = dynamicManager.getConfig(key);
if (!StringUtils.isEmpty(rawConfig)) {
process(new ConfigChangedEvent(key, "sofa-rpc",rawConfig));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure listeners are unregistered to prevent memory leaks

In the initWith method, the ConsumerAttributeListener registers itself with the DynamicConfigManager:

dynamicManager.addListener(key, this);

However, there is no corresponding logic to remove this listener when it is no longer needed. This might lead to memory leaks if the listener remains registered after the consumer is unreferenced.

Consider implementing a mechanism to unregister the listener when the consumer is unreferenced or destroyed. This can be done in the unRefer method by calling a hypothetical removeListener method:

dynamicManager.removeListener(key, this);


@Override
public void process(ConfigChangedEvent event){
if (event.getChangeType().equals(ConfigChangeType.DELETED)) {
newValueMap = null;
} else {
// ADDED or MODIFIED
String[] lines = event.getContent().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();
newValueMap.put(key, value);
}
}
}
attrUpdated(newValueMap);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Add error handling for configuration parsing in process method

In the process method, the configuration content is split and parsed without validating the input:

String[] lines = event.getContent().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();
        newValueMap.put(key, value);
    }
}

If the configuration content is malformed, this could lead to incorrect behavior or missed configurations.

Consider adding error handling to manage malformed lines gracefully. For example, log a warning when a line doesn't conform to the expected key=value format:

for (String line : lines) {
    String[] keyValue = line.split("=", 2);
    if (keyValue.length == 2) {
        String key = keyValue[0].trim();
        String value = keyValue[1].trim();
        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
83 changes: 79 additions & 4 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.dynamic.*;
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,11 +45,30 @@
@Extension(value = "apollo", override = true)
public class ApolloDynamicConfigManager extends DynamicConfigManager {

Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify Access Modifier and Make LOGGER Static Final

The LOGGER variable is declared without an access modifier and is not static or final. It's a best practice to declare loggers as private static final to ensure they are class-level constants and not modifiable.

Apply this diff to correct it:

- Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);
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
Logger LOGGER = LoggerFactory.getLogger(ApolloDynamicConfigManager.class);
private static final 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_PROTOCOL_PREFIX = "http://";

private Config config;

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

protected ApolloDynamicConfigManager(String appName) {
super(appName);
config = ConfigService.getAppConfig();
System.setProperty(APOLLO_APPID_KEY, appName);
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + SofaConfigs.getOrDefault(DynamicConfigKeys.APOLLO_ADDRESS));
config = ConfigService.getConfig(DynamicConfigKeys.DEFAULT_GROUP);
}

protected ApolloDynamicConfigManager(String appName,String address) {
super(appName);
System.setProperty(APOLLO_APPID_KEY, appName);
System.setProperty(APOLLO_ADDR_KEY, APOLLO_PROTOCOL_PREFIX + address);
config = ConfigService.getConfig(DynamicConfigKeys.DEFAULT_GROUP);
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid Using System.setProperty in Constructors

Setting system properties within a constructor can have unintended side effects, as system properties are global to the JVM and affect all parts of the application. This may lead to unpredictable behavior, especially in multi-threaded environments or when multiple instances are created.

Consider refactoring the code to avoid setting system properties in the constructor. Instead, pass the necessary configuration directly to the components that require it.

}

@Override
Expand Down Expand Up @@ -77,4 +107,49 @@ public AuthRuleGroup getServiceAuthRule(String service) {
//TODO 暂不支持
return null;
}

@Override
public String getConfig(String key){
return config.getProperty(key, DynamicHelper.DEFAULT_DYNAMIC_VALUE);
}

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

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);
if ("".equals(change.getNewValue())) {
LOGGER.info("an empty rule is received for key: " + key);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect Use of return Inside Loop

Using return inside the loop in the onChange method will exit the entire method when an empty new value is encountered. This prevents processing of subsequent keys, which may lead to missed configuration changes.

Apply this diff to fix the issue:

- if ("".equals(change.getNewValue())) {
    LOGGER.info("an empty rule is received for key: " + key);
-   return;
+   continue;
}
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
if ("".equals(change.getNewValue())) {
LOGGER.info("an empty rule is received for key: " + key);
return;
if ("".equals(change.getNewValue())) {
LOGGER.info("an empty rule is received for key: " + key);
continue;
}

}

ConfigChangedEvent event =
new ConfigChangedEvent(key, change.getNamespace(), change.getNewValue(), getChangeType(change));
listeners.forEach(listener -> listener.process(event));
}
}

private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
}
return ConfigChangeType.MODIFIED;
}
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

Handle All Possible Property Change Types

The getChangeType method currently handles only DELETED and defaults all other change types to MODIFIED. However, there may be other change types like ADDED. Explicitly handling all possible PropertyChangeType values ensures correct behavior and easier maintenance.

Update the method to handle additional change types:

private ConfigChangeType getChangeType(ConfigChange change) {
    if (change.getChangeType() == PropertyChangeType.DELETED) {
        return ConfigChangeType.DELETED;
+   } else if (change.getChangeType() == PropertyChangeType.ADDED) {
+       return ConfigChangeType.ADDED;
    }
    return ConfigChangeType.MODIFIED;
}
📝 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;
}
return ConfigChangeType.MODIFIED;
}
private ConfigChangeType getChangeType(ConfigChange change) {
if (change.getChangeType() == PropertyChangeType.DELETED) {
return ConfigChangeType.DELETED;
} else if (change.getChangeType() == PropertyChangeType.ADDED) {
return ConfigChangeType.ADDED;
}
return ConfigChangeType.MODIFIED;
}


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

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure Thread Safety When Managing Listeners

The listeners set in the ApolloListener class uses CopyOnWriteArraySet for thread safety. While this is suitable for scenarios with infrequent mutations and frequent iterations, it may not be the most efficient choice if listeners are added or removed frequently.

Evaluate whether ConcurrentHashMap or another thread-safe collection would be more appropriate, especially if listener additions and removals occur often.

}
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