Improve concurrent request bucketing in queue-proxy. #1091
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
google-prow-robot
added
the
size/M
cmd/queue/main.go
Outdated
| reqInChan = make(chan queue.Poke, requestCountingQueueLength) | ||
| reqOutChan = make(chan queue.Poke, requestCountingQueueLength) | ||
| reqInChan = make(chan queue.Poke) | ||
| reqOutChan = make(chan queue.Poke) |
There was a problem hiding this comment.
I'm fairly new to go so please bear with me: My understanding is, that we shouldn't buffer here anymore. In an extreme case, this could lead to reporting a higher concurrency than what we actually see in the container (since In and Out are different channels). Maybe it makes sense instead to move away from "Poke" but have "In" and "Out" and push those through the same channel?
There was a problem hiding this comment.
By not having a buffer the side effect is http requests that are being proxied through the queue will block waiting for something to read from this channel. Looking at the code this would likely happen when sending stats to the autoscaler is on a slow network.
It might be worth thinking about queue.Poke be or contain a timestamp. Then when aggregating we check if the time falls within our bucket interval and maybe drop the poke if it's not.
There was a problem hiding this comment.
@dprotaso the channel for sending stats is still buffered though. My understanding is, this only makes incrementing/decrementing the concurrency counter blocking, which might be okay?
There was a problem hiding this comment.
I don't think we should allow request handling to block on stat reporting. So I think the buffered channel should remain. We could push both in and out through the same channel, although I don't think it makes much of a difference. Order is not critical here.
We can think about what happens when stat reporting gets way behind. Right now, concurrency "happens" when the stat reporter sees it. The problem with making the Poke a Timestamp is that we have to keep in and out balanced. Otherwise concurrency won't return to zero. E.g. if we disregard one "in" because it's late, we must remember to disregard one "out". Which one?
|
/assign @josephburnett |
|
@markusthoemmes need to sign a CLA |
|
I signed the CLA! |
|
CLAs look good, thanks! |
google-prow-robot
added
size/L
cmd/queue/main.go
Outdated
| reqInChan = make(chan queue.Poke, requestCountingQueueLength) | ||
| reqOutChan = make(chan queue.Poke, requestCountingQueueLength) | ||
| reqInChan = make(chan queue.Poke) | ||
| reqOutChan = make(chan queue.Poke) |
There was a problem hiding this comment.
I don't think we should allow request handling to block on stat reporting. So I think the buffered channel should remain. We could push both in and out through the same channel, although I don't think it makes much of a difference. Order is not critical here.
We can think about what happens when stat reporting gets way behind. Right now, concurrency "happens" when the stat reporter sees it. The problem with making the Poke a Timestamp is that we have to keep in and out balanced. Otherwise concurrency won't return to zero. E.g. if we disregard one "in" because it's late, we must remember to disregard one "out". Which one?
Instead of counting all requests that arrived in a certain bucket as concurrent, the queue proxy now reports the actual maximum concurrency that was present inside of one bucket. If for example 3 requests arrive at once, the maximum concurrency will be 3 for that bucket. If another arises while the 3 remaining are still open, maximum concurrency is 4. Closing requests results in a decrement on the concurrency immediatly (versus draining the outgoing request queue on quantization which results in the behavior described above). Closes #1060
|
/retest |
pkg/queue/stats.go
Outdated
| // Ticks with every request completed | ||
| ReqOutChan chan Poke | ||
| // Ticks with every request arrived/completed respectively | ||
| ReqChan chan interface{} |
There was a problem hiding this comment.
Nit: chan interface{} seems too open to me. You can throw anything on the channel and the compiler won't tell you there's a problem. But binary will crash when the switch statement has no match.
How about an enumerated type instead?
type StatEvent int
const (
ReqIn StatEvent = iota
ReqOut
)
ReqChan chan ReqEvent
switch event {
case ReqIn:
case ReqOut:
}
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: josephburnett, markusthoemmes, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-prow-robot
added
the
approved
Fixes #1060
Proposed Changes
Instead of counting all requests that arrived in a certain bucket as concurrent, the queue proxy now reports the actual maximum concurrency that was present inside of one bucket.
If for example 3 requests arrive at once, the maximum concurrency will be 3 for that bucket. If another arises while the 3 remaining are still open, maximum concurrency is 4. Closing requests results in a decrement on the concurrency immediatly (versus draining the outgoing request queue on quantization which results in the behavior described above).
Release Note