[ISSUE #4735] SubscriptionManager enhancement #4736
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4736 +/- ##
============================================
+ Coverage 16.37% 17.55% +1.17%
- Complexity 1734 1778 +44
============================================
Files 856 797 -59
Lines 31265 29866 -1399
Branches 2701 2581 -120
============================================
+ Hits 5120 5243 +123
+ Misses 25665 24140 -1525
- Partials 480 483 +3 ☔ View full report in Codecov by Sentry. |
|
This PR mainly focuses on the thread safety of SubscriptionManager. |
|
@karsonto please fix the conflicts thanks. |
karsonto
force-pushed
the
develop-subscriptionManager-enhancement
branch
from
2976b25 to
1278952
Compare
Please help to review again,thank you. |
|
plz resolve conflicts |
| isContains = true; | ||
| localClient.setLastUpTime(new Date()); | ||
| break; | ||
| lock.lock(); |
There was a problem hiding this comment.
The thread-safety issue with the method is brought about by the global variable. Could you explain where there is a thread safety issue with the way the original method uses ConcurrentHashMap?
该方法的线程安全问题是由全局变量带来的。能否解释下原方法对ConcurrentHashMap的使用方式哪里存在线程安全问题？
There was a problem hiding this comment.
这个改动主要消除了SubscribeProcessor 使用 SubscriptionManager synchronized 方法，保证了SubscriptionManager方法调用是线程安全的，是线程安全的类
| @@ -56,6 +55,10 @@ public class SubscriptionManager { | |||
| */ | |||
| private final ConcurrentHashMap<String, List<Client>> localClientInfoMapping = new ConcurrentHashMap<>(64); | |||
|
|
|||
| private final ConcurrentHashMap<String, Client> urlClientInfoMapping = new ConcurrentHashMap<>(64); | |||
There was a problem hiding this comment.
Could you clarify what the role of this global variable is? I only see one role for it: to store the newly created Client in the registerClient() method. and that role doesn't seem to have an advantage compared to the way the original method handles it, but rather adds overhead to the global variable.
您能否说明下该全局变量的作用是什么？我只看到它的一个作用：在registerClient()方法存储新创建的Client。而该作用相比较原方法的处理方式好像并无优势，反而增加了全局变量的开销。
There was a problem hiding this comment.
添加全局变量保存url 以及client对应关系，消除了每次注册client时的遍历，提高了注册效率
There was a problem hiding this comment.
Do you mean eliminating the second for(){} in registerClient() method? If so, it doesn't actually eliminate the traversal. List#contains() is also a traversal operation internally. And there are problems with that traversal, as I explained to you.
您的意思是消除了registerClient()方法中第二个的for(){}吗？如果是的话，实际上并没有消除该遍历。List#contains()内部也是遍历操作。而且该遍历存在的问题，我也向您说明了。
There was a problem hiding this comment.
那用Set 方式代替List 你觉得可行吗？
There was a problem hiding this comment.
@karsonto
I think switching to Set might improve iterating speed. But the way it compares elements when iterating has the same problem as List#contains().
我认为改用Set也许能提高遍历速度。但是它遍历中比较元素的方式也存在和List#contains()一样的问题。
There was a problem hiding this comment.
如果改用Set集合，重写Client url hash code方法，加入到集合里就不需要再遍历。
There was a problem hiding this comment.
If the value in localClientInfoMapping is changed to Set, does the equals method need to be rewritten in addition to the hashCode method?
如果将localClientInfoMapping中的value改成Set，除了hashCode方法，equals方法是否也需要重写？
There was a problem hiding this comment.
是的，使用HashSet 代替ArrayList 需要重写Client hash code 以及equals 方法，用单一url属性判断防重复
| return client; | ||
| }); | ||
| clientInner.reNewLastUpTime(); | ||
| if (!localClients.contains(clientInner)) { |
There was a problem hiding this comment.
This type of judgment is inconsistent with the original method logic. Your way will determine if they are equal or not by Client#equals(), i.e. memory address. Whereas the original method logic is comparing the content of the Client, i.e. the url attribute (at least).
这种判断方式和原方法逻辑是不一致的。您的方式将通过Client#equals()判断是否相等，即内存地址。而原方法逻辑是比较Client的内容，即url属性（至少）。
There was a problem hiding this comment.
@karsonto It seems that this conversation is waiting for your reply.
There was a problem hiding this comment.
@karsonto Hi, I still haven't seen your reply. If you have already "replied", please check if the comment is in "pending" status. Thank you.
karsonto
force-pushed
the
develop-subscriptionManager-enhancement
branch
from
1278952 to
0d9e9d6
Compare
Pil0tXia
added
the
waiting for contributor
|
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
The main changes are as follows:
SubscriptionManager add a reentrant lock ensure thread safety
SubscriptionManager adds url client mapping
Optimize registerClient logic and remove the inefficient method of finding objects in for loop
SubscribeProcessor and LocalSubscribeEventProcessor remove the synchronized method calling SubscriptionManager
Fixes #4735
Motivation
Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.
Modifications
Describe the modifications you've done.
Documentation