optimize: redis registry push empty protection optimize #6164

laywin · 2023-12-16T05:57:58Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

当收到 redis 注册中心回调的服务器下线消息时，检查服务器地址是否是地址列表中唯一的地址并且回调的集群是当前使用的集群，如果是开启空推保护，否则将该服务器地址移除掉.
when receive server offline message from redis registry check the serverAddr is unique in the address list and
the callback cluster is current cluster, then enable push-empty protection
Otherwise, remove it.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2023-12-16T06:04:57Z

Codecov Report

Merging #6164 (5749426) into 2.x (501d431) will increase coverage by 0.04%.
Report is 1 commits behind head on 2.x.
The diff coverage is 46.66%.

❗ Current head 5749426 differs from pull request most recent head fca1868. Consider uploading reports for the commit fca1868 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #6164      +/-   ##
============================================
+ Coverage     48.98%   49.03%   +0.04%     
- Complexity     4782     4797      +15     
============================================
  Files           913      915       +2     
  Lines         31697    31861     +164     
  Branches       3823     3842      +19     
============================================
+ Hits          15528    15622      +94     
- Misses        14632    14684      +52     
- Partials       1537     1555      +18

Files	Coverage Δ
...n/java/io/seata/core/rpc/netty/ChannelManager.java	`0.00% <ø> (ø)`
...va/io/seata/integration/tx/api/util/DubboUtil.java	`0.00% <0.00%> (ø)`
...overy/registry/redis/RedisRegistryServiceImpl.java	`67.28% <50.00%> (ø)`

... and 6 files with indirect coverage changes

slievrly · 2023-12-16T10:21:09Z

...iscovery-redis/src/main/java/io/seata/discovery/registry/redis/RedisRegistryServiceImpl.java

+        if (socketAddresses.size() == 1 && socketAddresses.contains(inetSocketAddress)) {
+            String txServiceGroupName = ConfigurationFactory.getInstance()
+                    .getConfig(ConfigurationKeys.TX_SERVICE_GROUP);
+            String clusterName = getServiceGroup(txServiceGroupName);


check txServiceGroupName if null.

看了一下，当 txServiceGroupName 是null的时候也没问题，真实环境，txServiceGroupName 基本不可能为空吧.
i see no problem for the situation when txServiceGroupName is null, but in real environment, txServiceGroupName almost impossible to be null.

What I mean is that if txServiceGroupName is null, you should not use it as a parameter to get the clusterName.

Already optimized it.

funky-eyes · 2023-12-17T10:24:42Z

...iscovery-redis/src/main/java/io/seata/discovery/registry/redis/RedisRegistryServiceImpl.java

+        if (socketAddresses.size() == 1 && socketAddresses.contains(inetSocketAddress)) {
+            String txServiceGroupName = ConfigurationFactory.getInstance()
+                    .getConfig(ConfigurationKeys.TX_SERVICE_GROUP);
+            String clusterName = getServiceGroup(txServiceGroupName);


已经有clustername了,为什么还要通过事务分组再次读取?
I already have clustername, why read it again through transaction grouping?

notifyCluserName 是注册中心回调通知的clusterName, 需要和当前正在使用的clusterName做对比.
notifyCluserName is the clusterName of the registration center callback notification, which needs to be compared with the clusterName currently in use.

clusterName

为什么要跟当前的做对比？而不是直接用他回调的去删除缓存中的？变化的是回调的那个cluster，如果你不需要改变，为什么不是取消对其的订阅？
Why compare it with the current one? Instead of directly using what he called back to delete the one in the cache? What changes is the cluster that called back. If you don't need to change it, why not unsubscribe to it?

funky-eyes · 2023-12-17T10:25:47Z

...iscovery-redis/src/main/java/io/seata/discovery/registry/redis/RedisRegistryServiceImpl.java

+
+        Set<InetSocketAddress> socketAddresses = CLUSTER_ADDRESS_MAP.getOrDefault(notifyCluserName, Collections.emptySet());
+        InetSocketAddress inetSocketAddress = NetUtil.toInetSocketAddress(serverAddr);
+        if (socketAddresses.size() == 1 && socketAddresses.contains(inetSocketAddress)) {


请考虑并发性问题
Please consider concurrency issues

… into redis_push_empty_protection

funky-eyes · 2023-12-20T01:24:04Z

...iscovery-redis/src/main/java/io/seata/discovery/registry/redis/RedisRegistryServiceImpl.java

+        public void accept(RedisMessage redisMessage) {
+            reentrantLock.lock();
+            try {
+                if (redisMessage.getMessageType() == MessageType.SUBSCRIBE) {


为什么messagetype不是add或者remove？而是scan和SUBSCRIBE？我希望是scan那部分找到的列表能跟当前的cluster中的集合做对比，将需要add或者remove的事件发出，而不是在这里面写ifelse，结果还是各玩各的。我希望这里只是一个动作触发，要么是remove，要么是add
Why is messagetype not add or remove? But scan and SUBSCRIBE? I hope that the list found in the scan part can be compared with the collection in the current cluster, and the events that need to be added or removed will be sent out, instead of writing ifelse in it, the result is still different. I hope this is just an action trigger, either remove or add.

codecov-commenter · 2023-12-20T02:31:04Z

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

…teIfAbsent, there is a performance problem in jdk8. 2. optimize unit test.

...very-redis/src/test/java/io/seata/discovery/registry/redis/RedisRegisterServiceImplTest.java

…ry/registry/redis/RedisRegisterServiceImplTest.java

…otection

funky-eyes

LGTM

laywin added 2 commits December 16, 2023 13:50

redis registry push empty protection optimize

85f0b0e

add change log

9911bb4

add unit test

5ed822a

slievrly reviewed Dec 16, 2023

View reviewed changes

laywin mentioned this pull request Dec 17, 2023

issue of zk push-empty protection #6162

Open

1 task

funky-eyes reviewed Dec 17, 2023

View reviewed changes

funky-eyes added 13 commits December 18, 2023 11:07

add test case

3449120

Merge branch '2.x' into redis_push_empty_protection

d98ad94

Merge branch 'redis_push_empty_protection' of github.com:laywin/seata…

8be6b42

… into redis_push_empty_protection

add test case

7ae2bd5

add test case

8e6d283

add test case

88ff43b

add test case

3846568

add test case

6f04d1d

add test case

56780ae

add test case

776497d

test

becbea9

test

5749426

test

d423833

funky-eyes reviewed Dec 20, 2023

View reviewed changes

optimize

85a4b72

laywin force-pushed the redis_push_empty_protection branch from 32837a3 to 85a4b72 Compare December 20, 2023 02:30

laywin added 2 commits December 20, 2023 10:57

1. replace ConcurrentHashMap.computeIfAbsent to CollectionUtils.compu…

2cc328b

…teIfAbsent, there is a performance problem in jdk8. 2. optimize unit test.

fix: check style

0424055

funky-eyes reviewed Dec 20, 2023

View reviewed changes

...very-redis/src/test/java/io/seata/discovery/registry/redis/RedisRegisterServiceImplTest.java Outdated Show resolved Hide resolved

1. optimize unit test

c845405

funky-eyes reviewed Dec 21, 2023

View reviewed changes

...very-redis/src/test/java/io/seata/discovery/registry/redis/RedisRegisterServiceImplTest.java Outdated Show resolved Hide resolved

funky-eyes closed this Dec 21, 2023

funky-eyes reopened this Dec 21, 2023

funky-eyes reviewed Dec 21, 2023

View reviewed changes

...very-redis/src/test/java/io/seata/discovery/registry/redis/RedisRegisterServiceImplTest.java Outdated Show resolved Hide resolved

funky-eyes added this to the 2.1.0 milestone Dec 21, 2023

funky-eyes added module/discovery discovery module type: optimize labels Dec 21, 2023

funky-eyes and others added 2 commits December 21, 2023 09:35

Merge branch '2.x' into redis_push_empty_protection

912c0e5

optimize unit test

d51cba5

funky-eyes reviewed Dec 21, 2023

View reviewed changes

...very-redis/src/test/java/io/seata/discovery/registry/redis/RedisRegisterServiceImplTest.java Outdated Show resolved Hide resolved

funky-eyes and others added 2 commits December 21, 2023 11:24

Update discovery/seata-discovery-redis/src/test/java/io/seata/discove…

c31f3f0

…ry/registry/redis/RedisRegisterServiceImplTest.java

Merge branch '2.x' of github.com:seata/seata into redis_push_empty_pr…

30e68c6

…otection

funky-eyes approved these changes Dec 21, 2023

View reviewed changes

funky-eyes merged commit ab474eb into apache:2.x Dec 21, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize: redis registry push empty protection optimize #6164

optimize: redis registry push empty protection optimize #6164

laywin commented Dec 16, 2023 •

edited

Loading

codecov bot commented Dec 16, 2023 •

edited

Loading

slievrly Dec 16, 2023

laywin Dec 16, 2023

slievrly Dec 17, 2023

laywin Dec 19, 2023

funky-eyes Dec 17, 2023

laywin Dec 17, 2023

funky-eyes Dec 18, 2023

funky-eyes Dec 17, 2023

funky-eyes Dec 20, 2023

codecov-commenter commented Dec 20, 2023

funky-eyes left a comment

optimize: redis registry push empty protection optimize #6164

optimize: redis registry push empty protection optimize #6164

Conversation

laywin commented Dec 16, 2023 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Dec 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 20, 2023

Welcome to Codecov 🎉

funky-eyes left a comment

Choose a reason for hiding this comment

laywin commented Dec 16, 2023 •

edited

Loading

codecov bot commented Dec 16, 2023 •

edited

Loading