Skip to content

Commit 607af9d

Browse files
beiwei30chickenlj
beiwei30
authored and
chickenlj
committed
Merge pull request apache#2817, code review for dubbo-configcente.
1 parent 6a1b9ee commit 607af9d

File tree

6 files changed

+83
-91
lines changed

6 files changed

+83
-91
lines changed

‎dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java

+11-4
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import org.apache.dubbo.common.URL;
2121
import org.apache.dubbo.common.Version;
2222
import org.apache.dubbo.common.config.Environment;
23-
import org.apache.dubbo.common.extension.ExtensionLoader;
2423
import org.apache.dubbo.common.utils.CollectionUtils;
2524
import org.apache.dubbo.common.utils.ConfigUtils;
2625
import org.apache.dubbo.common.utils.NetUtils;
@@ -46,6 +45,7 @@
4645
import java.util.Set;
4746

4847
import static org.apache.dubbo.common.Constants.APPLICATION_KEY;
48+
import static org.apache.dubbo.common.extension.ExtensionLoader.getExtensionLoader;
4949

5050
/**
5151
* AbstractDefaultConfig
@@ -154,10 +154,17 @@ protected void checkRegistry() {
154154
}
155155
}
156156

157-
// For compatibility purpose, use registry as the default config center if the registry protocol is zookeeper and there's no config center specified explicitly.
157+
useRegistryForConfigIfNecessary();
158+
}
159+
160+
/**
161+
* For compatibility purpose, use registry as the default config center if the registry protocol is zookeeper and
162+
* there's no config center specified explicitly.
163+
*/
164+
private void useRegistryForConfigIfNecessary() {
158165
RegistryConfig registry = registries.get(0);
159166
if (registry.isZookeeperProtocol()) {
160-
Set<DynamicConfiguration> loadedConfigurations = ExtensionLoader.getExtensionLoader(DynamicConfiguration.class).getExtensions();
167+
Set<DynamicConfiguration> loadedConfigurations = getExtensionLoader(DynamicConfiguration.class).getExtensions();
161168
// we use the loading status of DynamicConfiguration to decide whether ConfigCenter has been initiated.
162169
if (CollectionUtils.isEmpty(loadedConfigurations)) {
163170
ConfigCenterConfig configCenterConfig = new ConfigCenterConfig();
@@ -298,7 +305,7 @@ protected URL loadMonitor(URL registryURL) {
298305
}
299306
if (ConfigUtils.isNotEmpty(address)) {
300307
if (!map.containsKey(Constants.PROTOCOL_KEY)) {
301-
if (ExtensionLoader.getExtensionLoader(MonitorFactory.class).hasExtension("logstat")) {
308+
if (getExtensionLoader(MonitorFactory.class).hasExtension("logstat")) {
302309
map.put(Constants.PROTOCOL_KEY, "logstat");
303310
} else {
304311
map.put(Constants.PROTOCOL_KEY, "dubbo");

‎dubbo-configcenter/dubbo-configcenter-api/src/main/java/org/apache/dubbo/configcenter/ConfigurationListener.java

-5
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@
1616
*/
1717
package org.apache.dubbo.configcenter;
1818

19-
import org.apache.dubbo.common.URL;
20-
2119
/**
2220
* Config listener, will get notified when the config it listens on changes.
2321
*/
@@ -30,7 +28,4 @@ public interface ConfigurationListener {
3028
* @param event config change event
3129
*/
3230
void process(ConfigChangeEvent event);
33-
34-
// FIXME: why we need this?
35-
URL getUrl();
3631
}

‎dubbo-configcenter/dubbo-configcenter-apollo/src/main/java/org/apache/dubbo/configcenter/support/apollo/ApolloDynamicConfiguration.java

+31-42
Original file line numberDiff line numberDiff line change
@@ -22,22 +22,24 @@
2222
import com.ctrip.framework.apollo.enums.ConfigSourceType;
2323
import com.ctrip.framework.apollo.enums.PropertyChangeType;
2424
import com.ctrip.framework.apollo.model.ConfigChange;
25-
import com.ctrip.framework.apollo.model.ConfigChangeEvent;
25+
2626
import org.apache.dubbo.common.Constants;
2727
import org.apache.dubbo.common.URL;
2828
import org.apache.dubbo.common.logger.Logger;
2929
import org.apache.dubbo.common.logger.LoggerFactory;
3030
import org.apache.dubbo.common.utils.StringUtils;
3131
import org.apache.dubbo.configcenter.AbstractDynamicConfiguration;
32+
import org.apache.dubbo.configcenter.ConfigChangeEvent;
3233
import org.apache.dubbo.configcenter.ConfigChangeType;
33-
import org.apache.dubbo.configcenter.ConfigType;
3434
import org.apache.dubbo.configcenter.ConfigurationListener;
3535

36-
import java.util.HashSet;
37-
import java.util.Set;
36+
import java.util.Collections;
37+
38+
import static org.apache.dubbo.configcenter.ConfigType.CONFIGURATORS;
39+
import static org.apache.dubbo.configcenter.ConfigType.ROUTERS;
3840

3941
/**
40-
*
42+
* Apollo implementation, https://github.com/ctripcorp/apollo
4143
*/
4244
public class ApolloDynamicConfiguration extends AbstractDynamicConfiguration<ConfigChangeListener> {
4345
private static final Logger logger = LoggerFactory.getLogger(ApolloDynamicConfiguration.class);
@@ -54,9 +56,8 @@ public ApolloDynamicConfiguration() {
5456
@Override
5557
public void initWith(URL url) {
5658
super.initWith(url);
57-
/**
58-
* Instead of using Dubbo's configuration, I would suggest use the original configuration method Apollo provides.
59-
*/
59+
60+
// Instead of using Dubbo's configuration, I would suggest use the original configuration method Apollo provides.
6061
String configEnv = url.getParameter(Constants.CONFIG_ENV_KEY);
6162
String configAddr = url.getBackupAddress();
6263
String configCluster = url.getParameter(Constants.CONFIG_CLUSTER_KEY);
@@ -75,24 +76,20 @@ public void initWith(URL url) {
7576
boolean check = url.getParameter(Constants.CONFIG_CHECK_KEY, false);
7677
if (dubboConfig.getSourceType() != ConfigSourceType.REMOTE) {
7778
if (check) {
78-
throw new IllegalStateException("Failed to connect to ConfigCenter, the ConfigCenter is Apollo, the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv));
79+
throw new IllegalStateException("Failed to connect to config center, the config center is Apollo, " +
80+
"the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv));
7981
} else {
80-
logger.warn("Failed to connect to ConfigCenter, the ConfigCenter is Apollo, " +
82+
logger.warn("Failed to connect to config center, the config center is Apollo, " +
8183
"the address is: " + (StringUtils.isNotEmpty(configAddr) ? configAddr : configEnv) +
82-
". will use the local cache value instead before finally connected.");
84+
", will use the local cache value instead before eventually the connection is established.");
8385
}
8486
}
8587
}
8688

8789
/**
88-
* This method will being used to,
90+
* This method will be used to:
8991
* 1. get configuration file at startup phase
9092
* 2. get all kinds of Dubbo rules
91-
*
92-
* @param key
93-
* @param group
94-
* @param timeout
95-
* @return
9693
*/
9794
@Override
9895
protected String getTargetConfig(String key, String group, long timeout) {
@@ -110,9 +107,6 @@ protected String getTargetConfig(String key, String group, long timeout) {
110107
* This method will used by Configuration to get valid value at runtime.
111108
* The group is expected to be 'app level', which can be fetched from the 'config.appnamespace' in url if necessary.
112109
* But I think Apollo's inheritance feature of namespace can solve the problem .
113-
*
114-
* @param key
115-
* @return
116110
*/
117111
@Override
118112
protected String getInternalProperty(String key) {
@@ -121,52 +115,47 @@ protected String getInternalProperty(String key) {
121115

122116
@Override
123117
protected void addTargetListener(String key, ConfigChangeListener listener) {
124-
Set<String> keys = new HashSet<>(1);
125-
keys.add(key);
126-
this.dubboConfig.addChangeListener(listener, keys);
118+
this.dubboConfig.addChangeListener(listener, Collections.singleton(key));
127119
}
128120

129121
@Override
130122
protected ConfigChangeListener createTargetListener(String key, ConfigurationListener listener) {
131123
return new ApolloListener(listener);
132124
}
133125

134-
public ConfigChangeType getChangeType(ConfigChange change) {
135-
if (change.getChangeType() == PropertyChangeType.DELETED) {
136-
return ConfigChangeType.DELETED;
137-
}
138-
return ConfigChangeType.MODIFIED;
139-
}
140-
141126
private class ApolloListener implements ConfigChangeListener {
142-
private ConfigurationListener listener;
143-
private URL url;
144127

145-
public ApolloListener(ConfigurationListener listener) {
146-
this(listener.getUrl(), listener);
147-
}
128+
private ConfigurationListener listener;
148129

149-
public ApolloListener(URL url, ConfigurationListener listener) {
150-
this.url = url;
130+
ApolloListener(ConfigurationListener listener) {
151131
this.listener = listener;
152132
}
153133

154134
@Override
155-
public void onChange(ConfigChangeEvent changeEvent) {
135+
public void onChange(com.ctrip.framework.apollo.model.ConfigChangeEvent changeEvent) {
156136
for (String key : changeEvent.changedKeys()) {
157137
ConfigChange change = changeEvent.getChange(key);
158138
if (StringUtils.isEmpty(change.getNewValue())) {
159-
logger.warn("We received an empty rule for " + key + ", the current working rule is " + change.getOldValue() + ", the empty rule will not take effect.");
139+
logger.warn("an empty rule is received for " + key + ", the current working rule is " +
140+
change.getOldValue() + ", the empty rule will not take effect.");
160141
return;
161142
}
162-
// TODO Maybe we no longer need to identify the type of change. Because there's no scenario that a callback will subscribe for both configurators and routers
143+
// TODO Maybe we no longer need to identify the type of change. Because there's no scenario that
144+
// a callback will subscribe for both configurators and routers
163145
if (change.getPropertyName().endsWith(Constants.CONFIGURATORS_SUFFIX)) {
164-
listener.process(new org.apache.dubbo.configcenter.ConfigChangeEvent(key, change.getNewValue(), ConfigType.CONFIGURATORS, getChangeType(change)));
146+
listener.process(new ConfigChangeEvent(key, change.getNewValue(), CONFIGURATORS, getChangeType(change)));
165147
} else {
166-
listener.process(new org.apache.dubbo.configcenter.ConfigChangeEvent(key, change.getNewValue(), ConfigType.ROUTERS, getChangeType(change)));
148+
listener.process(new ConfigChangeEvent(key, change.getNewValue(), ROUTERS, getChangeType(change)));
167149
}
168150
}
169151
}
152+
153+
private ConfigChangeType getChangeType(ConfigChange change) {
154+
if (change.getChangeType() == PropertyChangeType.DELETED) {
155+
return ConfigChangeType.DELETED;
156+
}
157+
return ConfigChangeType.MODIFIED;
158+
}
170159
}
171160

172161
}

‎dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/archaius/ArchaiusDynamicConfiguration.java

+20-20
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,16 @@
3232
import org.apache.dubbo.configcenter.ConfigurationListener;
3333
import org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource;
3434

35+
import static org.apache.dubbo.common.Constants.CONFIG_NAMESPACE_KEY;
36+
import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_CHECK_KEY;
37+
import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_ROOT_PATH_KEY;
38+
import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.ARCHAIUS_SOURCE_ADDRESS_KEY;
39+
import static org.apache.dubbo.configcenter.support.archaius.sources.ZooKeeperConfigurationSource.DEFAULT_CONFIG_ROOT_PATH;
40+
3541
/**
3642
* Archaius supports various sources and it's extensiable: JDBC, ZK, Properties, ..., so should we make it extensiable?
43+
* FIXME: we should get rid of Archaius or move it to eco system since Archaius is out of maintenance, instead, we
44+
* should rely on curator-x-async which looks quite promising.
3745
*/
3846
public class ArchaiusDynamicConfiguration extends AbstractDynamicConfiguration<Runnable> {
3947
private static final Logger logger = LoggerFactory.getLogger(ArchaiusDynamicConfiguration.class);
@@ -50,10 +58,10 @@ public void initWith(URL url) {
5058

5159
String address = url.getBackupAddress();
5260
if (!address.equals(Constants.ANYHOST_VALUE)) {
53-
System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_SOURCE_ADDRESS_KEY, address);
61+
System.setProperty(ARCHAIUS_SOURCE_ADDRESS_KEY, address);
5462
}
55-
System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_ROOT_PATH_KEY, url.getParameter(Constants.CONFIG_NAMESPACE_KEY, ZooKeeperConfigurationSource.DEFAULT_CONFIG_ROOT_PATH));
56-
System.setProperty(ZooKeeperConfigurationSource.ARCHAIUS_CONFIG_CHECK_KEY, url.getParameter(Constants.CONFIG_CHECK_KEY, "false"));
63+
System.setProperty(ARCHAIUS_CONFIG_ROOT_PATH_KEY, url.getParameter(CONFIG_NAMESPACE_KEY, DEFAULT_CONFIG_ROOT_PATH));
64+
System.setProperty(ARCHAIUS_CONFIG_CHECK_KEY, url.getParameter(Constants.CONFIG_CHECK_KEY, "false"));
5765

5866
try {
5967
ZooKeeperConfigurationSource zkConfigSource = new ZooKeeperConfigurationSource(url);
@@ -71,15 +79,11 @@ public void initWith(URL url) {
7179
/**
7280
* The hierarchy of configuration properties is:
7381
* 1. /{namespace}/config/dubbo/dubbo.properties
74-
* 2. /{namespace}/config/applicationname/dubbo.properties
82+
* 2. /{namespace}/config/{applicationname}/dubbo.properties
7583
* <p>
76-
* To make the API compatible with other configuration systems, the key doesn't has group as prefix, so we should add the group prefix before try to get value.
77-
* If being used for dubbo router rules, the key must already contains group prefix.
78-
*
79-
* @param key
80-
* @param group
81-
* @param timeout
82-
* @return
84+
* To make the API compatible with other configuration systems, the key doesn't has group as prefix, so we should
85+
* add the group prefix before try to get value. If being used for dubbo router rules, the key must already
86+
* contains group prefix.
8387
*/
8488
@Override
8589
protected String getTargetConfig(String key, String group, long timeout) {
@@ -93,11 +97,7 @@ protected String getTargetConfig(String key, String group, long timeout) {
9397
}
9498

9599
/**
96-
* First, get app level configuration
97-
* If there's no value in app level, try to get global dubbo level.
98-
*
99-
* @param key
100-
* @return
100+
* First, get app level configuration. If there's no value in app level, try to get global dubbo level.
101101
*/
102102
@Override
103103
protected Object getInternalProperty(String key) {
@@ -120,15 +120,14 @@ protected Runnable createTargetListener(String key, ConfigurationListener listen
120120

121121
private class ArchaiusListener implements Runnable {
122122
private ConfigurationListener listener;
123-
private URL url;
124123
private String key;
125124
private ConfigType type;
126125

127126
public ArchaiusListener(String key, ConfigurationListener listener) {
128127
this.key = key;
129128
this.listener = listener;
130-
this.url = listener.getUrl();
131-
// Maybe we no longer need to identify the type of change. Because there's no scenario that a callback will subscribe for both configurators and routers
129+
// Maybe we no longer need to identify the type of change. Because there's no scenario that a callback
130+
// will subscribe for both configurators and routers
132131
if (key.endsWith(Constants.CONFIGURATORS_SUFFIX)) {
133132
type = ConfigType.CONFIGURATORS;
134133
} else {
@@ -152,7 +151,8 @@ public void run() {
152151
listener.process(event);
153152
} else {
154153
if (newValue.equals("")) {
155-
logger.warn("We received an empty rule for " + key + ", the current working rule is unknown, the empty rule will not take effect.");
154+
logger.warn("an empty rule is received for " + key + ", the current working rule is unknown, " +
155+
"the empty rule will not take effect.");
156156
return;
157157
}
158158
event.setChangeType(ConfigChangeType.MODIFIED);

0 commit comments

Comments
 (0)