Pass a context to the query worker pool #3350
Conversation
This ensures requests that have already timed out don't stick around waiting for a worker to complete the request.
Codecov Report
@@ Coverage Diff @@
## master #3350 +/- ##
=========================================
- Coverage 72.3% 72.3% -0.1%
=========================================
Files 1098 1098
Lines 102107 102109 +2
=========================================
- Hits 73903 73898 -5
- Misses 23103 23106 +3
- Partials 5101 5105 +4
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
Mainly looks good to me although i do worry about what happens when you have 10,000 series and how much lock contention/overhead will happen checking the context for every single one of them - you kind of want to check the context say “every N” series to amortize that cost ideally (although, how complicated is that, can that be achieved by changing which worker pool method to call every N series? shrug) |
The fast wrappers allow a caller to amortize the cost of checking the Done channel over a batchSize of iterations.
src/query/storage/prom_converter.go
Outdated
| @@ -101,6 +103,7 @@ func toPromConcurrently( | |||
| mu sync.Mutex | |||
| ) | |||
|
|
|||
| fastWorkerPool := readWorkerPool.Fast(100) | |||
There was a problem hiding this comment.
nit: should we make this Java-like and more explicit what you get back here, i.e. make the method calledFastGoWithContextWorkerPool? Otherwise it's not clear that only the GoWithContext is fast (and the other methods are the same).
There was a problem hiding this comment.
FastGoWithContextWorkerPoolFactoryProviderBean ? j/k will change.
There was a problem hiding this comment.
LGTM other than the nit:
https://github.com/m3db/m3/pull/3350/files#r604571533
This ensures requests that have already timed out don't stick around
waiting for a worker to complete the request.
What this PR does / why we need it:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: