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,28 @@ | |||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||
| 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.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 +49,72 @@ | |||||||||||||||||||||||||||||||||||||||||||
| @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_PARAM_APPID_KEY = "appId"; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| private static final String APOLLO_PARAM_CLUSTER_KEY = "cluster"; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| private static final String APOLLO_PROTOCOL_PREFIX = "http://"; | ||||||||||||||||||||||||||||||||||||||||||||
| private final ConcurrentMap<String, ApolloListener> watchListenerMap = new ConcurrentHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||
| private Config config; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| protected ApolloDynamicConfigManager(String appName) { | ||||||||||||||||||||||||||||||||||||||||||||
| 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()); | ||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Avoid hardcoding protocol when setting Currently, the code appends 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());
}
|
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if (StringUtils.isNotBlank(getDynamicUrl().getParam(APOLLO_PARAM_CLUSTER_KEY))) { | ||||||||||||||||||||||||||||||||||||||||||||
| System.setProperty(APOLLO_CLUSTER_KEY, getDynamicUrl().getParam(APOLLO_PARAM_CLUSTER_KEY)); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| 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 +123,43 @@ public AuthRuleGroup getServiceAuthRule(String service) { | |||||||||||||||||||||||||||||||||||||||||||
| //TODO 暂不支持 | ||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||
| public void addListener(String key, ConfigListener listener) { | ||||||||||||||||||||||||||||||||||||||||||||
| ApolloListener apolloListener = watchListenerMap.computeIfAbsent(key, k -> new ApolloListener()); | ||||||||||||||||||||||||||||||||||||||||||||
| apolloListener.addListener(listener); | ||||||||||||||||||||||||||||||||||||||||||||
| config.addChangeListener(apolloListener, Collections.singleton(key)); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+146
to
+150
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; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if (change.getChangeType() == PropertyChangeType.ADDED) { | ||||||||||||||||||||||||||||||||||||||||||||
| return ConfigChangeType.ADDED; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| return ConfigChangeType.MODIFIED; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+166
to
+174
There was a problem hiding this comment. 🛠️ Refactor suggestion Handle All Possible Change Types Explicitly In the 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
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.
Add error handling for request-level dynamic configuration initialization.
The initialization of request-level dynamic configuration lacks error handling. If
getDynamicManagerreturns null orinitServiceConfigurationfails, 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