fix(receive): resolve 503 errors during ingestor restarts#8720
Conversation
- Make Dialer interface context-aware (DialContext) so TCP connections respect forwarding timeouts - Move sendWrites into a goroutine to unblock the response collection loop - Return codes.DeadlineExceeded instead of codes.Unavailable for context-originated failures in Cap'n Proto client - Replace WorkerPool.Go(Work) with Go(ctx, Work) error to support context cancellation on saturated pools Signed-off-by: Pavel Litvyak <[email protected]>
c1804c0 to
9ac9c18
Compare
| } | ||
| } | ||
|
|
||
| if ctx.Err() != nil { |
There was a problem hiding this comment.
Should we use errors.Is(err, context.DeadlineExceeded) instead?
There was a problem hiding this comment.
Yep, good point! We certainly should use errors.Is here and seems like we even can handle context.DeadlineExceeded and context.Canceled separately - but I'm not sure if it's really important here.
| }) | ||
| }); err != nil { | ||
| responseWriter <- newWriteResponse( | ||
| seriesIDs, |
There was a problem hiding this comment.
Should we also update attributes on the trace like above?
There was a problem hiding this comment.
Sure, I'll take a look how to add the properly in this case
| h.sendWrites(ctx, &wg, params, localWrites, remoteWrites, responses) | ||
|
|
||
| go func() { | ||
| h.sendWrites(ctx, &wg, params, localWrites, remoteWrites, responses) |
There was a problem hiding this comment.
Should we remove the sync.WaitGroup if we are now calling this in the same goroutine? At least I don't see a purpose for it anymore.
There was a problem hiding this comment.
I think that we can't remove it since sendRemoteWrite dispatches work via RemoteWriteAsync, so the actual writes complete asynchronously and we need to track when all those async callbacks finish, so we know when to close(responses)
There was a problem hiding this comment.
Actually this line is the source of a backpressure regression. sendWrites runs sendLocalWrite synchronously inline — moving it into a goroutine takes the local TSDB Append off the request's critical path. Once quorum can be satisfied from remote acks alone, the handler returns success while local Append is still pending, gRPC admits the next request, and wrapper goroutines accumulate without admission control. Lock contention on TSDB → slower local Append → more goroutines stacking up — a negative feedback loop. We correlated this against prod heap/CPU profiles.
There was a problem hiding this comment.
Hi @jnyi
I think your comment is relevant. I tested this PR in my own environment but mostly did it with a split routers/ingesters topology, seems like it's the reason why I didn't mention this behavior.
Am I right that you noticed this regression on a default mode?
There was a problem hiding this comment.
Hmm, I'm trying to think of a good solution here. Maybe we can just convert local writes into a worker architecture as well so that they would all look uniform? That way we wouldn't spawn more go routines if the local appends are lagging up to a certain extent.
There was a problem hiding this comment.
Trying to address this in #8866. I feel like this change/refactor made the code also easier to understand since remote/local writes goes through the same infra. Let me know if you have any comments.
- Use errors.Is for context error checking in capnp client, mapping DeadlineExceeded and Canceled to appropriate gRPC codes - Add unit tests for RemoteWrite gRPC error code mapping - Add tracing span with error attributes when worker pool rejects task Signed-off-by: Pavel Litvyak <[email protected]>
3 participants
Changes
sendWritesinto a goroutine to unblock the response collection loopcodes.DeadlineExceededinstead ofcodes.Unavailablefor context-originated failures in Cap'n Proto clientWorkerPool.Go(Work)withGo(ctx, Work) errorto support context cancellation on saturated poolsCloses #8710
Verification
I've tested it in our test environment with separate routers/ingestors, RF=2 and Cap'n Proto - and it looks good. However it certainly needs a thoughtful review.