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

[Dubbo-4525] fix Clusterutils.mergeurl, make sure specific keys from provider side not override by that from consumer. #4533

Merged
merged 4 commits into from
Jul 12, 2019
Merged

[Dubbo-4525] fix Clusterutils.mergeurl, make sure specific keys from provider side not override by that from consumer. #4533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67f8912
fix url merge bug, keep remote url param
chickenlj Jul 11, 2019
9031d7b
fix UT, add version & group to the url value when specifying remote u…
chickenlj Jul 11, 2019
1213e13
refactor ClusterUtils
chickenlj Jul 11, 2019
6f20c5e
remove unused imports
chickenlj Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 16 additions & 37 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/support/ClusterUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@
package org.apache.dubbo.rpc.cluster.support;

import org.apache.dubbo.common.URL;
import org.apache.dubbo.common.utils.StringUtils;
import org.apache.dubbo.remoting.Constants;

import java.util.HashMap;
import java.util.Map;

import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.ALIVE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CORE_THREADS_KEY;
Expand All @@ -41,6 +39,7 @@
import static org.apache.dubbo.remoting.Constants.DUBBO_VERSION_KEY;
import static org.apache.dubbo.rpc.Constants.INVOKER_LISTENER_KEY;
import static org.apache.dubbo.rpc.Constants.REFERENCE_FILTER_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;

/**
* ClusterUtils
Expand Down Expand Up @@ -81,55 +80,35 @@ public static URL mergeUrl(URL remoteUrl, Map<String, String> localMap) {
}

if (localMap != null && localMap.size() > 0) {
// All providers come to here have been filtered by group, which means only those providers that have the exact same group value with the consumer could come to here.
// So, generally, we don't need to care about the group value here.
// But when comes to group merger, there is an exception, the consumer group may be '*' while the provider group can be empty or any other values.
String remoteGroup = map.get(GROUP_KEY);
String remoteRelease = map.get(RELEASE_KEY);
map.putAll(localMap);
if (StringUtils.isNotEmpty(remoteGroup)) {
map.put(GROUP_KEY, remoteGroup);
}
// we should always keep the Provider RELEASE_KEY not overrode by the the value on Consumer side.
map.remove(RELEASE_KEY);
if (StringUtils.isNotEmpty(remoteRelease)) {
map.put(RELEASE_KEY, remoteRelease);
}
}
if (remoteMap != null && remoteMap.size() > 0) {
// Use version passed from provider side
reserveRemoteValue(DUBBO_VERSION_KEY, map, remoteMap);
reserveRemoteValue(VERSION_KEY, map, remoteMap);
reserveRemoteValue(METHODS_KEY, map, remoteMap);
reserveRemoteValue(TIMESTAMP_KEY, map, remoteMap);
reserveRemoteValue(TAG_KEY, map, remoteMap);
// TODO, for compatibility consideration, we cannot simply change the value behind APPLICATION_KEY from Consumer to Provider. So just add an extra key here.
// Reserve application name from provider.
Map<String, String> copyOfLocalMap = new HashMap<>(localMap);
copyOfLocalMap.remove(GROUP_KEY);
copyOfLocalMap.remove(RELEASE_KEY);
copyOfLocalMap.remove(DUBBO_VERSION_KEY);
copyOfLocalMap.remove(VERSION_KEY);
copyOfLocalMap.remove(METHODS_KEY);
copyOfLocalMap.remove(TIMESTAMP_KEY);
copyOfLocalMap.remove(TAG_KEY);

map.putAll(copyOfLocalMap);

map.put(REMOTE_APPLICATION_KEY, remoteMap.get(APPLICATION_KEY));

// Combine filters and listeners on Provider and Consumer
String remoteFilter = remoteMap.get(REFERENCE_FILTER_KEY);
String localFilter = localMap.get(REFERENCE_FILTER_KEY);
String localFilter = copyOfLocalMap.get(REFERENCE_FILTER_KEY);
if (remoteFilter != null && remoteFilter.length() > 0
&& localFilter != null && localFilter.length() > 0) {
localMap.put(REFERENCE_FILTER_KEY, remoteFilter + "," + localFilter);
map.put(REFERENCE_FILTER_KEY, remoteFilter + "," + localFilter);
}
String remoteListener = remoteMap.get(INVOKER_LISTENER_KEY);
String localListener = localMap.get(INVOKER_LISTENER_KEY);
String localListener = copyOfLocalMap.get(INVOKER_LISTENER_KEY);
if (remoteListener != null && remoteListener.length() > 0
&& localListener != null && localListener.length() > 0) {
localMap.put(INVOKER_LISTENER_KEY, remoteListener + "," + localListener);
map.put(INVOKER_LISTENER_KEY, remoteListener + "," + localListener);
}
}

return remoteUrl.clearParameters().addParameters(map);
}

private static void reserveRemoteValue(String key, Map<String, String> map, Map<String, String> remoteMap) {
String remoteValue = remoteMap.get(key);
if (StringUtils.isNotEmpty(remoteValue)) {
map.put(key, remoteValue);
}
}

}
74 changes: 73 additions & 1 deletion dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/ClusterUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,27 @@
import org.junit.jupiter.api.Test;

import static org.apache.dubbo.common.constants.CommonConstants.ALIVE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CLUSTER_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CORE_THREADS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.DEFAULT_KEY_PREFIX;
import static org.apache.dubbo.common.constants.CommonConstants.DUBBO_PROTOCOL;
import static org.apache.dubbo.common.constants.CommonConstants.GROUP_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.METHODS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.PID_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.QUEUES_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.RELEASE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.REMOTE_APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREADPOOL_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREADS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREAD_NAME_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.TIMEOUT_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.TIMESTAMP_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.VERSION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.DUBBO_PROTOCOL;
import static org.apache.dubbo.remoting.Constants.DUBBO_VERSION_KEY;
import static org.apache.dubbo.rpc.Constants.REFERENCE_FILTER_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.LOADBALANCE_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;

public class ClusterUtilsTest {

Expand All @@ -60,11 +70,15 @@ public void testMergeUrl() throws Exception {
.addParameter(DEFAULT_KEY_PREFIX + QUEUES_KEY, Integer.MAX_VALUE)
.addParameter(DEFAULT_KEY_PREFIX + ALIVE_KEY, Integer.MAX_VALUE)
.addParameter(DEFAULT_KEY_PREFIX + THREAD_NAME_KEY, "test")
.addParameter(APPLICATION_KEY, "provider")
.addParameter(REFERENCE_FILTER_KEY, "filter1,filter2")
.build();

URL consumerURL = new URLBuilder(DUBBO_PROTOCOL, "localhost", 55555)
.addParameter(PID_KEY, "1234")
.addParameter(THREADPOOL_KEY, "foo")
.addParameter(APPLICATION_KEY, "consumer")
.addParameter(REFERENCE_FILTER_KEY, "filter3")
.build();

URL url = ClusterUtils.mergeUrl(providerURL, consumerURL.getParameters());
Expand All @@ -91,6 +105,64 @@ public void testMergeUrl() throws Exception {
Assertions.assertEquals(url.getPassword(), "password");
Assertions.assertEquals(url.getParameter(PID_KEY), "1234");
Assertions.assertEquals(url.getParameter(THREADPOOL_KEY), "foo");
Assertions.assertEquals(url.getParameter(APPLICATION_KEY), "consumer");
Assertions.assertEquals(url.getParameter(REMOTE_APPLICATION_KEY), "provider");
Assertions.assertEquals(url.getParameter(REFERENCE_FILTER_KEY), "filter1,filter2,filter3");
}

@Test
public void testUseProviderParams() {
// present in both local and remote, but uses remote value.
URL localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
URL remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=remote&group=remote&dubbo=remote&release=remote" +
"&methods=remote&tag=remote&timestamp=remote");
URL mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertEquals(remoteURL.getParameter(VERSION_KEY), mergedUrl.getParameter(VERSION_KEY));
Assertions.assertEquals(remoteURL.getParameter(GROUP_KEY), mergedUrl.getParameter(GROUP_KEY));
Assertions.assertEquals(remoteURL.getParameter(DUBBO_VERSION_KEY), mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertEquals(remoteURL.getParameter(RELEASE_KEY), mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertEquals(remoteURL.getParameter(METHODS_KEY), mergedUrl.getParameter(METHODS_KEY));
Assertions.assertEquals(remoteURL.getParameter(TIMESTAMP_KEY), mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertEquals(remoteURL.getParameter(TAG_KEY), mergedUrl.getParameter(TAG_KEY));

// present in local url but not in remote url, parameters of remote url is empty
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertNull(mergedUrl.getParameter(VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(GROUP_KEY));
Assertions.assertNull(mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertNull(mergedUrl.getParameter(METHODS_KEY));
Assertions.assertNull(mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertNull(mergedUrl.getParameter(TAG_KEY));

// present in local url but not in remote url
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?key=value");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertNull(mergedUrl.getParameter(VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(GROUP_KEY));
Assertions.assertNull(mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertNull(mergedUrl.getParameter(METHODS_KEY));
Assertions.assertNull(mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertNull(mergedUrl.getParameter(TAG_KEY));

// present in both local and remote, uses local url params
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?loadbalance=local&timeout=1000&cluster=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?loadbalance=remote&timeout=2000&cluster=remote");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertEquals(localURL.getParameter(CLUSTER_KEY), mergedUrl.getParameter(CLUSTER_KEY));
Assertions.assertEquals(localURL.getParameter(TIMEOUT_KEY), mergedUrl.getParameter(TIMEOUT_KEY));
Assertions.assertEquals(localURL.getParameter(LOADBALANCE_KEY), mergedUrl.getParameter(LOADBALANCE_KEY));
}

}
6 changes: 3 additions & 3 deletions ...ubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
@TestPropertySource(properties = {
"packagesToScan = org.apache.dubbo.config.spring.context.annotation.provider",
"consumer.version = ${demo.service.version}",
"consumer.url = dubbo://127.0.0.1:12345",
"consumer.url = dubbo://127.0.0.1:12345?version=2.5.7",
})
public class ReferenceAnnotationBeanPostProcessorTest {

Expand Down Expand Up @@ -221,7 +221,7 @@ public DemoService getDemoServiceFromAncestor() {
return demoServiceFromAncestor;
}

@Reference(id = "my-reference-bean", version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(id = "my-reference-bean", version = "2.5.7", url = "dubbo://127.0.0.1:12345?version=2.5.7")
public void setDemoServiceFromAncestor(DemoService demoServiceFromAncestor) {
this.demoServiceFromAncestor = demoServiceFromAncestor;
}
Expand Down Expand Up @@ -259,7 +259,7 @@ public DemoService getDemoService() {
return demoService;
}

@com.alibaba.dubbo.config.annotation.Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@com.alibaba.dubbo.config.annotation.Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345?version=2.5.7")
public void setDemoService(DemoService demoService) {
this.demoService = demoService;
}
Expand Down
10 changes: 6 additions & 4 deletions ...ava/org/apache/dubbo/config/spring/context/annotation/consumer/ConsumerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
@PropertySource("META-INF/default.properties")
public class ConsumerConfiguration {

private static final String remoteURL = "dubbo://127.0.0.1:12345?version=2.5.7";

/**
* Current application configuration, to replace XML config:
* <prev>
Expand Down Expand Up @@ -67,7 +69,7 @@ public RegistryConfig registryConfig() {
@Autowired
private DemoService autowiredDemoService;

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoService;

public DemoService getDemoService() {
Expand All @@ -86,7 +88,7 @@ public Child c() {

public static abstract class Ancestor {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoServiceFromAncestor;

public DemoService getDemoServiceFromAncestor() {
Expand All @@ -106,7 +108,7 @@ public DemoService getDemoServiceFromParent() {
return demoServiceFromParent;
}

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
public void setDemoServiceFromParent(DemoService demoServiceFromParent) {
this.demoServiceFromParent = demoServiceFromParent;
}
Expand All @@ -118,7 +120,7 @@ public static class Child extends Parent {
@Autowired
private DemoService demoService;

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoServiceFromChild;


Expand Down
10 changes: 6 additions & 4 deletions ...pache/dubbo/config/spring/context/annotation/consumer/test/TestConsumerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
@EnableTransactionManagement
public class TestConsumerConfiguration {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
private static final String remoteURL = "dubbo://127.0.0.1:12345?version=2.5.7";

@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoService;

@Autowired
Expand All @@ -61,7 +63,7 @@ public TestConsumerConfiguration.Child c() {

public static abstract class Ancestor {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoServiceFromAncestor;

public DemoService getDemoServiceFromAncestor() {
Expand All @@ -81,7 +83,7 @@ public DemoService getDemoServiceFromParent() {
return demoServiceFromParent;
}

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
public void setDemoServiceFromParent(DemoService demoServiceFromParent) {
this.demoServiceFromParent = demoServiceFromParent;
}
Expand All @@ -90,7 +92,7 @@ public void setDemoServiceFromParent(DemoService demoServiceFromParent) {

public static class Child extends TestConsumerConfiguration.Parent {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoServiceFromChild;

public DemoService getDemoServiceFromChild() {
Expand Down