You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2021/08/15 12:22:16 UTC
[GitHub] [dubbo] Yanniiii opened a new pull request #8508: Fix issue 8506
Yanniiii opened a new pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508
## What is the purpose of the change
1.fix [#8506](https://github.com/apache/dubbo/issues/8506).
2.on the consumer side, deduplicate multiple copies of the same service instance from multiple registries to execute LB correctly.
## Brief changelog
## Verifying this change
<!-- Follow this checklist to help us incorporate your contribution quickly and easily: -->
## Checklist
- [x] Make sure there is a [GitHub_issue](https://github.com/apache/dubbo/issues) field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
- [ ] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in [dubbo samples](https://github.com/apache/dubbo-samples) project.
- [ ] Add some description to [dubbo-website](https://github.com/apache/dubbo-website) project if you are requesting to add a feature.
- [ ] GitHub Actions works fine on your own branch.
- [ ] If this contribution is large, please follow the [Software Donation Guide](https://github.com/apache/dubbo/wiki/Software-donation-guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#issuecomment-945379348
好的，我等下去改。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r690918511
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
I rebased the branch from 3.0 to Master.And maybe what you want to say is that these parameters are not enough to complete the deduplication correctly?
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
And I found that If I put the deduplication code in MultipleNotifyListenerWrapper.notify(), the code cannot pass the unit test in MultipleRegistry2S2RTest:212, because the parameters of NotifyListener.notify() are deduplicated, not the same as the parameters declared by the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] pinxiong commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
pinxiong commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r690959365
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
This is a multi-instance problem, and the community is discussing a reasonable solution to solve this issue. There are some thing duscussed in community, such as
+ How to define an unique instance
+ What resources need to be shared in a multi-instance scenario
+ How to define the lifecycle of the instance
If you are interested in this problem, welcome to join the metting!
+ Metting tool: [DingDing](https://www.dingtalk.com/)
+ Meeting Code: `864 912 87261`
+ Reference: [Agenda](https://www.yuque.com/apache-dubbo/dubbo3/wz70lz)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r690924387
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
And I found that If I put the deduplication code in MultipleNotifyListenerWrapper.notify(), the code cannot pass the unit test in MultipleRegistry2S2RTest:212, because the parameters of NotifyListener.notify() are deduplicated, not the same as the parameters declared by the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r696414062
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
Because different registries use different levels of URL classes, for example, the parameters in the URL object in Nacos have 3 more parameters than ZK, and even the number is different, so you can't arbitrarily compare each parameter. According to actual usage in development and tests, I feel that the 4 parameters of Address(host+port), Protocol, GroupID, VersionID are good keys for deduplication.
Thank you for your invitation, and when will the meeting start?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#issuecomment-947639098
改好了，但是根据我修改后的逻辑会有个2个测试通不过。那2个测试会把某服务，该服务只有一个实例，同时注册到zk和redis上，去重后应该会只保留一个URL，但是assert了2个。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] CrazyHZM closed pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
CrazyHZM closed pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] pinxiong commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
pinxiong commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r689197765
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -26,12 +26,7 @@
import org.apache.dubbo.registry.RegistryFactory;
import org.apache.dubbo.registry.support.AbstractRegistry;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
Review comment:
Remove the import*
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
It's not the best way to check if two URLs are equal, you can refer [URL#equals](https://github.com/apache/dubbo/blob/3.0/dubbo-common/src/main/java/org/apache/dubbo/common/URL.java#L1432)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] CrazyHZM commented on pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#issuecomment-948118053
@Yanniiii Someone raised a PR, please check again if there is a problem：#8899
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] CrazyHZM commented on pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#issuecomment-945134316
No feedback for a long time, there is any update to reopen the pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] CrazyHZM commented on pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
CrazyHZM commented on pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#issuecomment-944923343
Please resolve conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] Yanniiii commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
Yanniiii commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r690918511
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
I rebased the branch from 3.0 to Master.And maybe what you want to say is that these parameters are not enough to complete the deduplication correctly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] pinxiong commented on a change in pull request #8508: Fix issue 8506
Posted by GitBox <gi...@apache.org>.
pinxiong commented on a change in pull request #8508:
URL: https://github.com/apache/dubbo/pull/8508#discussion_r690959365
##########
File path: dubbo-registry/dubbo-registry-multiple/src/main/java/org/apache/dubbo/registry/multiple/MultipleRegistry.java
##########
@@ -200,9 +195,27 @@ public void unsubscribe(URL url, NotifyListener listener) {
urls.addAll(tmpUrls);
}
}
+ urls = removeDuplicate(urls);
return urls;
}
+ protected static List<URL> removeDuplicate(List<URL> urls) {
+ Set<List<String>> set = new HashSet<>();
+ List<URL> dedupList = new ArrayList<>();
+ for (URL url : urls) {
+ List<String> tmp = new ArrayList<>();
+ tmp.add(url.getAddress());
+ tmp.add(url.getProtocol());
+ tmp.add(url.getParameter("group"));
+ tmp.add(url.getParameter("version"));
+ if (!set.contains(tmp)) {
+ set.add(tmp);
+ dedupList.add(url);
+ }
Review comment:
This is a multi-instance problem, and the community is discussing a reasonable solution to solve this issue. There are some thing duscussed in community, such as
+ How to define an unique instance
+ What resources need to be shared in a multi-instance scenario
+ How to define the lifecycle of the instance
If you are interested in this problem, welcome to join the metting!
+ Metting tool: [DingDing](https://www.dingtalk.com/)
+ Meeting Code: `864 912 87261`
+ Reference: [Agenda](https://www.yuque.com/apache-dubbo/dubbo3/wz70lz)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org