MDEV-33070 Thread pool starvation at oversubscribe#4804
MDEV-33070 Thread pool starvation at oversubscribe#4804varundeepsaini wants to merge 1 commit intoMariaDB:mainfrom
Conversation
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>
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.
| --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.
| 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".
| @_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.
| @@ -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.
| @@ -0,0 +1 @@ | |||
| --thread-handling=pool-of-threads | |||
There was a problem hiding this comment.
Please add --loose-thread-pool-mode=generic. We want that to also be tested on Windows (in its non-default thread-pool-mode).
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)
3 participants
Summary
Testing