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

client: fix the pd client could be blocked in some cases (#3283) #3285

Merged
merged 3 commits into from
Dec 23, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #3283 to release-4.0


Signed-off-by: leoppro zhaoyilin@pingcap.com

What problem does this PR solve?

Fix the PD client could be blocked in some cases

What is changed and how it works?

There are two levels of context in PD client: client context and tso request context.
the tsoRequest.Wait() function could be blocked when the client context is canceled because the Wait() function haven't listened the client context

Benchmark

the version of PD server: v4.0.8

before this PR:

Start benchmark #0, duration: 5s
Create 3 client(s) for benchmark
count:768290, max:9, min:0, avg:0, >1ms:34372, >2ms:2037, >5ms:457, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:800952, max:9, min:0, avg:0, >1ms:96750, >2ms:13593, >5ms:1495, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:745906, max:8, min:0, avg:0, >1ms:12877, >2ms:3652, >5ms:1330, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:759058, max:4, min:0, avg:0, >1ms:11232, >2ms:5776, >5ms:0, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0

Total:
count:3074206, max:9, min:0, avg:0, >1ms:155231, >2ms:25058, >5ms:3282, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:3074206, >1ms:5.05%, >2ms:0.82%, >5ms:0.11%, >10ms:0.00%, >30ms:0.00% >50ms:0.00% >100ms:0.00% >200ms:0.00% >400ms:0.00% >800ms:0.00% >1s:0.00%

after this PR:

Start benchmark #0, duration: 5s
Create 3 client(s) for benchmark
count:1361814, max:7, min:0, avg:1, >1ms:900605, >2ms:204456, >5ms:2896, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:1357623, max:11, min:0, avg:1, >1ms:931611, >2ms:151281, >5ms:3936, >10ms:1, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:737217, max:7, min:0, avg:0, >1ms:3830, >2ms:3444, >5ms:1028, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:1033124, max:8, min:0, avg:0, >1ms:430725, >2ms:83313, >5ms:460, >10ms:0, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0

Total:
count:4489778, max:11, min:0, avg:1, >1ms:2266771, >2ms:442494, >5ms:8320, >10ms:1, >30ms:0 >50ms:0 >100ms:0 >200ms:0 >400ms:0 >800ms:0 >1s:0
count:4489778, >1ms:50.49%, >2ms:9.86%, >5ms:0.19%, >10ms:0.00%, >30ms:0.00% >50ms:0.00% >100ms:0.00% >200ms:0.00% >400ms:0.00% >800ms:0.00% >1s:0.00%

Check List

Tests

  • Unit test

Release note

  • Fix the pd client could be blocked in some cases

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. type/cherry-pick labels Dec 17, 2020
@ti-srebot ti-srebot added this to the v4.0.9 milestone Dec 17, 2020
@ti-srebot
Copy link
Contributor Author

@leoppro please accept the invitation then you can push to the cherry-pick pull requests.
https://github.com/ti-srebot/pd/invitations

@JmPotato
Copy link
Member

@leoppro Please resolve the conflicts.

client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
@JmPotato
Copy link
Member

@leoppro It seems that applying suggestions doesn't have sign-off and misses fomating to the codes.

@rleungx rleungx modified the milestones: v4.0.9, v4.0.10 Dec 18, 2020
fmt
Signed-off-by: leoppro <zhaoyilin@pingcap.com>
@JmPotato
Copy link
Member

/lgtm

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #3285 (2dc9207) into release-4.0 (35e2d08) will decrease coverage by 0.09%.
The diff coverage is 75.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-4.0    #3285      +/-   ##
===============================================
- Coverage        74.68%   74.59%   -0.10%     
===============================================
  Files              209      209              
  Lines            19591    19594       +3     
===============================================
- Hits             14632    14616      -16     
- Misses            3633     3645      +12     
- Partials          1326     1333       +7     
Flag Coverage Δ
unittests 74.59% <75.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/client.go 69.03% <75.00%> (+0.23%) ⬆️
pkg/tempurl/tempurl.go 60.00% <0.00%> (-10.00%) ⬇️
pkg/etcdutil/etcdutil.go 76.47% <0.00%> (-5.89%) ⬇️
pkg/dashboard/adapter/manager.go 77.17% <0.00%> (-4.35%) ⬇️
server/member/leader.go 63.76% <0.00%> (-2.76%) ⬇️
server/api/member.go 52.25% <0.00%> (-2.71%) ⬇️
pkg/btree/btree.go 85.30% <0.00%> (-0.97%) ⬇️
server/server.go 73.16% <0.00%> (-0.84%) ⬇️
server/handler.go 44.27% <0.00%> (-0.50%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35e2d08...2dc9207. Read the comment docs.

@nolouch
Copy link
Contributor

nolouch commented Dec 23, 2020

/merge

@ti-chi-bot
Copy link
Member

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/ti-community-prow repository.

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 23, 2020
@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: f4e5e7f

@ti-chi-bot
Copy link
Member

@ti-srebot: Your PR has out-of-dated and I have automatically updated it for you.
At the same time I will also trigger all tests for you:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/ti-community-prow repository.

@ti-chi-bot ti-chi-bot merged commit 703471b into tikv:release-4.0 Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants