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

bugfix: the disposables tree set won't accept another Disposable with the same priority. #3170

Merged
merged 7 commits into from
Oct 20, 2020

Conversation

jujinghao
Copy link
Contributor

Fix the following issue:
Disposable TmNettyRemotingClient and RmNettyRemotingClient added by GlobalTransactionScanner have the same priority by default, so the RmNettyRemotingClient won't be closed gracefully during shutdown.

the same priority.

Disposable TmNettyRemotingClient and RmNettyRemotingClient added by
GlobalTransactionScanner have the same priority by default, so the
RmNettyRemotingClient won't be closed gracefully during shutdown.
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2020

Codecov Report

Merging #3170 into develop will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3170      +/-   ##
=============================================
- Coverage      50.38%   50.32%   -0.07%     
+ Complexity      3117     3114       -3     
=============================================
  Files            594      594              
  Lines          19625    19644      +19     
  Branches        2438     2443       +5     
=============================================
- Hits            9889     9885       -4     
- Misses          8743     8763      +20     
- Partials         993      996       +3     
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/seata/core/rpc/ShutdownHook.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...torage/file/store/FileTransactionStoreManager.java 56.77% <0.00%> (-0.65%) 28.00% <0.00%> (-1.00%)
...o/seata/server/coordinator/DefaultCoordinator.java 54.63% <0.00%> (-0.52%) 28.00% <0.00%> (-1.00%)
...in/java/io/seata/server/session/GlobalSession.java 82.43% <0.00%> (-0.46%) 70.00% <0.00%> (-1.00%)

@funky-eyes funky-eyes added first-time contributor first-time contributor module/core core module labels Oct 9, 2020
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #3170 into develop will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3170      +/-   ##
=============================================
- Coverage      50.95%   50.90%   -0.05%     
  Complexity      3205     3205              
=============================================
  Files            602      602              
  Lines          19815    19834      +19     
  Branches        2470     2475       +5     
=============================================
  Hits           10096    10096              
- Misses          8707     8726      +19     
  Partials        1012     1012              
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/seata/core/rpc/ShutdownHook.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM, great found.

@slievrly slievrly added this to the 1.4.0 milestone Oct 10, 2020
}

@Override
public int compareTo(DisposablePriorityWrapper disposablePriorityWrapper) {
return priority - disposablePriorityWrapper.priority;
Copy link
Contributor

Choose a reason for hiding this comment

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

use PriorityQueue to replace TreeSet will solve this problem once and forever. No need to add a "seqId" to fix the probem which is actually caused by TreeSet.

Comment on lines +111 to +119
if (cmp == 0) {
if (seqId > disposablePriorityWrapper.seqId) {
cmp = 1;
} else if (seqId < disposablePriorityWrapper.seqId) {
cmp = -1;
} else {
cmp = 0;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Long.compare(x, y) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Comment on lines +125 to +129
final int prime = 31;
int result = 1;
result = prime * result + priority;
result = prime * result + (int) (seqId ^ (seqId >>> 32));
return result;
Copy link
Member

Choose a reason for hiding this comment

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

Objects.hash(...values)?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Comment on lines +125 to +129
final int prime = 31;
int result = 1;
result = prime * result + priority;
result = prime * result + (int) (seqId ^ (seqId >>> 32));
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Comment on lines +111 to +119
if (cmp == 0) {
if (seqId > disposablePriorityWrapper.seqId) {
cmp = 1;
} else if (seqId < disposablePriorityWrapper.seqId) {
cmp = -1;
} else {
cmp = 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@slievrly slievrly merged commit 11ffe19 into apache:develop Oct 20, 2020
hicf pushed a commit to hicf/seata that referenced this pull request Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor module/core core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants