MDEV-33070 Thread pool starvation at oversubscribe#4804
MDEV-33070 Thread pool starvation at oversubscribe#4804varundeepsaini wants to merge 1 commit intoMariaDB:mainfrom
Conversation
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
mysql-test/main/mdev-33070.test
Outdated
| --source include/have_pool_of_threads.inc | ||
|
|
||
| let $extra_port=`SELECT @@port + 1`; | ||
| let $restart_parameters=--extra-port=$extra_port; |
There was a problem hiding this comment.
why don't you add this to the opt file and avoid the restart?
| @@ -0,0 +1,112 @@ | |||
| --source include/have_pool_of_threads.inc | |||
There was a problem hiding this comment.
we customarily start with a heading stating the MDEV.
mysql-test/main/mdev-33070.test
Outdated
| SET @@global.thread_pool_max_threads=@_thread_pool_max_threads, | ||
| @@global.thread_pool_size=@_thread_pool_size, | ||
| @@global.thread_pool_oversubscribe=@_thread_pool_oversubscribe, | ||
| @@global.thread_pool_stall_limit=@_thread_pool_stall_limit; |
There was a problem hiding this comment.
we customarily end with a footer of the type "End of ... tests".
mysql-test/main/mdev-33070.test
Outdated
| @_thread_pool_oversubscribe, | ||
| @_thread_pool_stall_limit; | ||
|
|
||
| SET @@global.thread_pool_max_threads=3, |
There was a problem hiding this comment.
I'd also add these to the .opt file.
mysql-test/main/mdev-33070.opt
Outdated
| @@ -0,0 +1 @@ | |||
| --thread-handling=pool-of-threads | |||
There was a problem hiding this comment.
Please add --loose-thread-pool-mode=generic. So that is also tested on Windows (with non-default thread-pool-mode)
| @@ -0,0 +1,112 @@ | |||
| --source include/have_pool_of_threads.inc | |||
There was a problem hiding this comment.
I think this test needs a good explanation.
To be honest, I don't quite understand, why we need thread_pool_max_threads=3, and 6 connections to prove that patch is working.
Would it work if we were more economic perhaps?
Say thread_pool_max_threads=2, oversubscribe=0, thread_pool_dedicated_listener=ON,
so we have one dedicated listener (so we can still enqueue new requests, into the queue), a single worker (making for 2 threads overall). And check that after killing a running query the queue is still drained by that single worker (i.e single worker won't switch to wait, because "too many threads" were active). Will this work and prove that the patch is correct? (I see that it is correct, but test case is not very obvious).
In any case, write the comments of how test case showing that the fix is working.
vaintroub
left a comment
vaintroub
left a comment
There was a problem hiding this comment.
Commented on test case. The one-liner code fix is ok, but test needs some improvement (and explanation)
varundeepsaini
force-pushed
the
MDEV-33070-thread-pool-starvation
branch
2 times, most recently
from
605117d to
21212ec
Compare
Allow one more worker at the oversubscribe threshold so a group only becomes oversubscribed after it exceeds the configured limit. Add a regression test that reproduces the starvation case in the generic thread pool and verifies queued work still drains. Signed-off-by: Varun Deep Saini <varun.23bcs10048@ms.sst.scaler.com>
varundeepsaini
force-pushed
the
MDEV-33070-thread-pool-starvation
branch
from
21212ec to
3b1f419
Compare
3 participants
Summary
Testing