Distributor: start remote timeout on first callback #6972
Conversation
It may take a while to calculate which instance gets each one of the series (see: grafana/dskit#454) It isn't fair (and is useless) to reach the callback with no ttl. This changes the code to start the timeout once the first callback is called. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
pkg/distributor/distributor.go
Outdated
| localCtx := context.WithoutCancel(ctx) | ||
| var cancelPushContext context.CancelFunc = func() {} | ||
| startRemoteTimeout := sync.OnceFunc(func() { | ||
| localCtx, cancelPushContext = context.WithTimeout(localCtx, d.cfg.RemoteTimeout) | ||
| }) |
There was a problem hiding this comment.
isn't there a race between swapping out the context in the first invocation and using a context in the next?
There was a problem hiding this comment.
maybe if you use sync.OnceValue() you can get around that
There was a problem hiding this comment.
Maybe split ring.DoBatchWithOptions into the slow init part and the make-calls part, so you don't need to change the variable in the callback?
There was a problem hiding this comment.
isn't there a race between swapping out the context in the first invocation and using a context in the next?
All calls will wait for first func to finish. See the comment on Once.Do:
func (o *Once) Do(f func()) {
// Note: Here is an incorrect implementation of Do:
//
// if atomic.CompareAndSwapUint32(&o.done, 0, 1) {
// f()
// }
//
// Do guarantees that when it returns, f has finished.
There was a problem hiding this comment.
I've refactored using sync.OnceValues to make it more obvious.
I had to move the fast slab pool to the parent context, which is passed to ring.DoBatch, but that shouldn't be an issue (the key is conserved, and I understand that it doesn't depend on the context cancellation, it only sets itself on the context).
I would love to refactor this entire method further, extracting smaller pieces of code into separate functions, and reordering, but I'd rather do that in a separate PR.
There was a problem hiding this comment.
Follow up PR with the refactor: #6978
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
| @@ -12,6 +12,7 @@ | |||
| * [ENHANCEMENT] Query-Frontend and Query-Scheduler: split tenant query request queues by query component with `query-frontend.additional-query-queue-dimensions-enabled` and `query-scheduler.additional-query-queue-dimensions-enabled`. #6772 | |||
| * [ENHANCEMENT] Store-gateway: include more information about lazy index-header loading in traces. #6922 | |||
| * [ENHANCEMENT] Distributor: support disabling metric relabel rules per-tenant via the flag `-distributor.metric-relabeling-enabled` or associated YAML. #6970 | |||
| * [ENHANCEMENT] Distributor: `-distributor.remote-timeout` is now accounted from the first ingester push request being sent. #6972 | |||
There was a problem hiding this comment.
Can user notice any difference in practice?
There was a problem hiding this comment.
If the number of series is really high, the preceding preparation code could consume the timeout before we even start sending network requests.
Maybe we could list this as a bugfix? 🤷
There was a problem hiding this comment.
Thanks for explanation. Bugfix or enhancement both make sense to me.
I wonder if we should have timeout for entire push request (incl. processing, this will especially become important for OTLP requests), although in fact there is one already -- -server.http-write-timeout. ("write" is unrelated to "push" in this case).
What this PR does
It may take a while to calculate which instance gets each one of the series (see: grafana/dskit#454)
It isn't fair (and is useless) to reach the callback with no ttl.
This changes the code to start the timeout once the first callback is called.
Which issue(s) this PR fixes or relates to
None.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.