Optimize dynamic config: integrate Zookeeper & Nacos, support interface-level dynamic config #1430
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,13 +16,23 @@ | |||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| 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.*; | ||||||||||||||||||||||||||||||||||
| 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.Set; | ||||||||||||||||||||||||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||||||||||||||||||||||||
| import java.util.concurrent.ConcurrentMap; | ||||||||||||||||||||||||||||||||||
| import java.util.concurrent.CopyOnWriteArraySet; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * @author bystander | ||||||||||||||||||||||||||||||||||
|
|
@@ -34,41 +44,83 @@ | |||||||||||||||||||||||||||||||||
| @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, 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()); | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Add a check for empty address before setting In the constructor, Consider adding a check to ensure 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
Suggested change
|
||||||||||||||||||||||||||||||||||
| String params[] = getParams(); | ||||||||||||||||||||||||||||||||||
| if (params!= null && params.length > 0){ | ||||||||||||||||||||||||||||||||||
| for (String param : params) { | ||||||||||||||||||||||||||||||||||
| String[] keyValue = param.split("="); | ||||||||||||||||||||||||||||||||||
| if (keyValue.length == 2) { | ||||||||||||||||||||||||||||||||||
| if ("cluster".equals(keyValue[0])) { | ||||||||||||||||||||||||||||||||||
| System.setProperty(APOLLO_CLUSTER_KEY, keyValue[1]); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| config = ConfigService.getAppConfig(); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public void initServiceConfiguration(String service) { | ||||||||||||||||||||||||||||||||||
| // TODO 暂不支持 | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+98
to
+99
There was a problem hiding this comment. Implement The method 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
Suggested change
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public void initServiceConfiguration(String service, ConfigListener listener) { | ||||||||||||||||||||||||||||||||||
| String rawConfig = config.getProperty(service, ""); | ||||||||||||||||||||||||||||||||||
| if (StringUtils.isNotBlank(rawConfig)) { | ||||||||||||||||||||||||||||||||||
| listener.process(new ConfigChangedEvent(service, rawConfig)); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Notify listener even when configuration is blank In the Apply this diff to notify the listener in all cases: if (StringUtils.isNotBlank(rawConfig)) {
listener.process(new ConfigChangedEvent(service, rawConfig));
+} else {
+ listener.process(new ConfigChangedEvent(service, rawConfig));
}Or simply remove the condition: -if (StringUtils.isNotBlank(rawConfig)) {
listener.process(new ConfigChangedEvent(service, rawConfig));
-}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public String getProviderServiceProperty(String service, String key) { | ||||||||||||||||||||||||||||||||||
| return config.getProperty(DynamicConfigKeyHelper.buildProviderServiceProKey(service, key), | ||||||||||||||||||||||||||||||||||
| DynamicHelper.DEFAULT_DYNAMIC_VALUE); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public String getConsumerServiceProperty(String service, String key) { | ||||||||||||||||||||||||||||||||||
| return config.getProperty(DynamicConfigKeyHelper.buildConsumerServiceProKey(service, key), | ||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public String getConsumerMethodProperty(String service, String method, String key) { | ||||||||||||||||||||||||||||||||||
| return config.getProperty(DynamicConfigKeyHelper.buildConsumerMethodProKey(service, method, key), | ||||||||||||||||||||||||||||||||||
| DynamicHelper.DEFAULT_DYNAMIC_VALUE); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
115
to
136
There was a problem hiding this comment. 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;
+ }
}
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -77,4 +129,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
There was a problem hiding this comment. 🛠️ 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 boolean hasListener(ConfigListener listener) {
return listeners.contains(listener);
}
boolean isEmpty() {
return listeners.isEmpty();
}
void removeListener(ConfigListener listener) {
listeners.remove(listener);
} |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public class ApolloListener implements ConfigChangeListener { | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private Set<ConfigListener> listeners = new CopyOnWriteArraySet<>(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| ApolloListener() { | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||
| public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) { | ||||||||||||||||||||||||||||||||||
| for (String key : changeEvent.changedKeys()) { | ||||||||||||||||||||||||||||||||||
| ConfigChange change = changeEvent.getChange(key); | ||||||||||||||||||||||||||||||||||
| ConfigChangedEvent event = | ||||||||||||||||||||||||||||||||||
| new ConfigChangedEvent(key, change.getNewValue(), getChangeType(change)); | ||||||||||||||||||||||||||||||||||
| listeners.forEach(listener -> listener.process(event)); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+157
to
+164
There was a problem hiding this comment. Prevent Concurrent Modification Exceptions Modifying the 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
Suggested change
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private ConfigChangeType getChangeType(ConfigChange change) { | ||||||||||||||||||||||||||||||||||
| if (change.getChangeType() == PropertyChangeType.DELETED) { | ||||||||||||||||||||||||||||||||||
| return ConfigChangeType.DELETED; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return ConfigChangeType.MODIFIED; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Handle All Possible Property Change Types The 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
Suggested change
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| void addListener(ConfigListener configListener) { | ||||||||||||||||||||||||||||||||||
| this.listeners.add(configListener); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+176
to
+178
There was a problem hiding this comment. 🛠️ Refactor suggestion Provide a method to remove listeners The Implement the void addListener(ConfigListener configListener) {
this.listeners.add(configListener);
}
+void removeListener(ConfigListener configListener) {
+ this.listeners.remove(configListener);
+}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -24,10 +26,11 @@ | |
|
|
||
| public class ApolloDynamicConfigManagerTest { | ||
|
|
||
| private final static Logger logger = LoggerFactory | ||
| .getLogger(ApolloDynamicConfigManagerTest.class); | ||
|
|
||
| private DynamicConfigManager apolloDynamicConfigManager = DynamicConfigManagerFactory.getDynamicManager("test", | ||
| "apollo"); | ||
|
|
||
| @Test | ||
| public void getProviderServiceProperty() { | ||
|
|
@@ -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() { | ||
| String result = apolloDynamicConfigManager.getConsumerMethodProperty("serviceName", "methodName", "timeout"); | ||
| Assert.assertEquals(DynamicHelper.DEFAULT_DYNAMIC_VALUE, result); | ||
|
Comment on lines
+43
to
+56
There was a problem hiding this comment. 🛠️ Refactor suggestion Enhance test coverage with additional test scenarios. The current tests only verify default values. Consider adding:
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");
} |
||
| } | ||
| } | ||
There was a problem hiding this comment.
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: