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.

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

optimize: RM TM startup connect server fail fast #6004

Merged
merged 40 commits into from
Jan 4, 2024
Merged

Conversation

jsbxyyx
Copy link
Member

@jsbxyyx jsbxyyx commented Nov 7, 2023

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

fixes: #5484

Ⅱ. 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

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #6004 (e549797) into 2.x (501d431) will decrease coverage by 0.12%.
Report is 1 commits behind head on 2.x.
The diff coverage is 31.88%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6004      +/-   ##
============================================
- Coverage     48.98%   48.87%   -0.12%     
+ Complexity     4782     4765      -17     
============================================
  Files           913      913              
  Lines         31697    31750      +53     
  Branches       3823     3835      +12     
============================================
- Hits          15528    15518      -10     
- Misses        14632    14689      +57     
- Partials       1537     1543       +6     
Files Coverage Δ
...c/main/java/io/seata/common/ConfigurationKeys.java 0.00% <ø> (ø)
...n/src/main/java/io/seata/common/DefaultValues.java 0.00% <ø> (ø)
...io/seata/core/rpc/netty/AbstractNettyRemoting.java 14.28% <ø> (ø)
...n/java/io/seata/core/rpc/netty/ChannelManager.java 0.00% <ø> (ø)
...ava/io/seata/core/rpc/netty/NettyClientConfig.java 44.77% <ø> (ø)
...io/seata/core/rpc/netty/TmNettyRemotingClient.java 55.00% <100.00%> (+0.45%) ⬆️
...va/io/seata/integration/tx/api/util/DubboUtil.java 0.00% <0.00%> (ø)
...io/seata/core/rpc/netty/RmNettyRemotingClient.java 37.31% <0.00%> (-0.57%) ⬇️
...ta/core/rpc/netty/AbstractNettyRemotingClient.java 18.26% <16.66%> (-0.45%) ⬇️
...rc/main/java/io/seata/common/util/StringUtils.java 40.37% <0.00%> (-4.12%) ⬇️
... and 1 more

... and 5 files with indirect coverage changes

@funky-eyes funky-eyes added this to the 2.x Backlog milestone Nov 11, 2023
funky-eyes
funky-eyes previously approved these changes Nov 27, 2023
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@laywin
Copy link
Contributor

laywin commented Nov 30, 2023

  1. may be fast fail is there are no available server from registry throw exception when start up, not there are available server but some can't connect. the check process can keep the available server list clear.
  2. In addition, i think the available server map can hook into registry, when the server not online, available server map can first time get the notify message not depend on 10s to polling.

@slievrly
Copy link
Member

slievrly commented Dec 6, 2023

  1. may be fast fail is there are no available server from registry throw exception when start up, not there are available server but some can't connect. the check process can keep the available server list clear.
  2. In addition, i think the available server map can hook into registry, when the server not online, available server map can first time get the notify message not depend on 10s to polling.

The server online/offline notifications in the registry center are based on a subscription mechanism. The "10s" mentioned here refers to the interval for checking the availability of servers. The registry center list displays the nodes that are deemed normal, but it doesn't guarantee that the clients can necessarily connect to them.

@laywin
Copy link
Contributor

laywin commented Dec 6, 2023

  1. may be fast fail is there are no available server from registry throw exception when start up, not there are available server but some can't connect. the check process can keep the available server list clear.
  2. In addition, i think the available server map can hook into registry, when the server not online, available server map can first time get the notify message not depend on 10s to polling.

The server online/offline notifications in the registry center are based on a subscription mechanism. The "10s" mentioned here refers to the interval for checking the availability of servers. The registry center list displays the nodes that are deemed normal, but it doesn't guarantee that the clients can necessarily connect to them.

Yes, I know the current situation. What I mean is that the list after check can also be referenced in the process of subscription monitoring to determine whether the node can be deleted at the first time when it goes offline, and 10s check is still in place.

是的,我知道现在的情况,我的意思是check 后的列表一样可以引用到订阅监听的过程中去,用来判断当节点下线时,能够第一时间删除掉, 10s check还在.

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2023

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (6011c68) 49.75% compared to head (0893fca) 49.96%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6004      +/-   ##
============================================
+ Coverage     49.75%   49.96%   +0.21%     
- Complexity     4885     4932      +47     
============================================
  Files           915      915              
  Lines         31885    31927      +42     
  Branches       3853     3855       +2     
============================================
+ Hits          15863    15952      +89     
+ Misses        14470    14411      -59     
- Partials       1552     1564      +12     
Files Coverage Δ
...c/main/java/io/seata/common/ConfigurationKeys.java 0.00% <ø> (ø)
...n/src/main/java/io/seata/common/DefaultValues.java 0.00% <ø> (ø)
...io/seata/core/rpc/netty/AbstractNettyRemoting.java 12.98% <ø> (-1.30%) ⬇️
...ava/io/seata/core/rpc/netty/NettyClientConfig.java 49.25% <ø> (+4.47%) ⬆️
...io/seata/core/rpc/netty/TmNettyRemotingClient.java 71.00% <100.00%> (+16.45%) ⬆️
...io/seata/core/rpc/netty/RmNettyRemotingClient.java 51.49% <25.00%> (+13.61%) ⬆️
...ta/core/rpc/netty/AbstractNettyRemotingClient.java 20.19% <16.66%> (+1.47%) ⬆️
...eata/core/rpc/netty/NettyClientChannelManager.java 62.17% <55.26%> (-0.33%) ⬇️
...rc/main/java/io/seata/common/util/StringUtils.java 46.00% <0.00%> (-2.59%) ⬇️

... and 11 files with indirect coverage changes

changes/en-us/2.x.md Outdated Show resolved Hide resolved
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@funky-eyes funky-eyes merged commit 2363715 into apache:2.x Jan 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support active check of service availability at client startup
6 participants