workerpool: remove mergeContext #2201
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
liuzix
added
the
subject/performance
ti-chi-bot
added
the
size/XL
liuzix
added
needs-cherry-pick-release-5.1
|
/run-integration-tests |
|
/run-leak-tests |
|
/run-leak-tests |
|
/run-leak-tests |
|
/run-integration-tests |
| }) | ||
| } | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Is it necessary to sleep 5s here?
There was a problem hiding this comment.
I just wanted to make sure that the internal queue in the pool has been fully congested. I can reduce it to 2s.
There was a problem hiding this comment.
We can loop and check.
for {
sleep(100ms)
if check_full? {
break
}
if wait_time > 5s {
panic("too long")
}
}
|
Could you move this issue forward? It have been tested for a while, not issue found. |
|
/run-unit-tests |
amyangfei
added
the
needs-cherry-pick-release-5.2
|
/run-integration-tests |
3 similar comments
|
/run-integration-tests |
|
/run-integration-tests |
|
/run-integration-tests |
|
/run-integration-tests /tikv=release-5.2 |
Codecov Report
@@ Coverage Diff @@
## master #2201 +/- ##
================================================
- Coverage 57.0620% 55.7204% -1.3416%
================================================
Files 165 168 +3
Lines 19435 20418 +983
================================================
+ Hits 11090 11377 +287
- Misses 7288 7938 +650
- Partials 1057 1103 +46 |
|
In response to a cherrypick label: new pull request created: #2486. |
|
In response to a cherrypick label: new pull request created: #2487. |
|
In response to a cherrypick label: new pull request created: #2488. |
|
In response to a cherrypick label: cannot checkout |
|
/run-cherry-picker |
|
/cherry-pick release-5.2 |
|
@amyangfei: new pull request created: #2490. In response to this:
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/tichi repository. |
What problem does this PR solve?
mergeContextspawns a goroutine whenDone()is called for the first time. Since we generate amergeContextfor each event before calling its handler, when concurrency is high, we will be running a large number of goroutines concurrently, and these goroutines do little that's useful.Close #2211
What is changed and how it works?
mergeContextcompletely. Event handler calls can only be cancelled with thectxused to add that event. They will no longer be cancelled when the workerpool is cancelled.Check List
Tests
Related changes
Release note