Remote execution: fix two cancellation-related issues #12856
Conversation
…es sent to the query-scheduler
charleskorn
force-pushed
the
charleskorn/fix-unnecessary-cancellations
branch
from
505c469 to
4a9102d
Compare
…ut before `protobufResponseDone` is closed
charleskorn
changed the title
I'm not clear on which components are being described here. A cancellation message being sent from query-frontends to query-schedulers after the response(?) had been read by queriers? Which response are queriers reading? |
pkg/frontend/v2/frontend.go
Outdated
| } | ||
|
|
||
| freq.spanLogger.DebugLog("msg", "request enqueued successfully, waiting for response") | ||
| <-ctx.Done() // The context will be cancelled if the request is cancelled by DoProtobufRequest's caller, if the response has been completely read, or if the stream is closed by the caller. |
There was a problem hiding this comment.
It took me a while to understand how this worked. Is it possible to expand this comment a little bit?
There was a problem hiding this comment.
How's this after my most recent changes?
Sorry, this had a typo in it, it should be: This PR mitigates a race condition where queries evaluated with remote execution could sometimes result in a cancellation message being sent to the query-scheduler after the response had been completely read by |
charleskorn
changed the title
…f the request is cancelled by the caller of `DoProtobufRequest`
charleskorn
changed the title
There was a problem hiding this comment.
This LGTM. Differentiating between requestContext, streamContext and the context given to ProtobufResponseStream.Next() really helped me understand what's going on here. Given the complexity here maybe @francoposa or @chencs could take a look? If they can't that's fine too.
|
@56quarters afk today but we never actually dealt with this end of the code for the scheduler project. Might be someone else that worked in code recently that would have a better opinion. |
# Conflicts: # CHANGELOG.md
Given your approval and the fact this is behind a feature flag, and the queue of other PRs waiting on this one, I'm going to merge this so I can start testing it in some test environments, but I'd love to hear any further feedback after I merge. |
#### What this PR does This PR implements support for query sharding inside MQE as an AST optimisation pass. There are some missing features that will be added in subsequent PRs: * we need to buffer query responses for sharded legs in query-frontends to avoid deadlocking if multiple query-frontends are evaluating shardable queries at the same time and the total number of legs exceeds the number of available querier workers: #12885 * we need to limit the number of sharded legs for a single query request evaluated at once, similar to the behaviour of the `limitedParallelismRoundTripper`: #12883 * we need to block user requests that use the `__sharded_concat__()`, as this may otherwise lead to unpredictable behaviour: #12886⚠️ The integration tests in this PR will only reliably pass with the fix from #12856. #### Which issue(s) this PR fixes or relates to (none) #### Checklist - [x] Tests updated. - [n/a] Documentation added. - [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. If changelog entry is not needed, please add the `changelog-not-needed` label to the PR. - [x] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features.
4 participants
What this PR does
This PR fixes two issues in the query-frontend:
Unnecessary request cancellation messages are sometimes sent to the query-scheduler
This PR fixes a race condition where queries evaluated with remote execution could sometimes result in a cancellation message being sent to the query-scheduler after the response had been completely read by query-frontends.
This issue had no user-visible impact, but it causes confusing messages in request traces, and in some cases can cause unnecessary shuffling of queriers if shuffle sharding is enabled. (This happens because cancelling the request causes the query-scheduler to close the querier/query-scheduler stream, and if all streams for a single querier are closed before any are re-established, then the querier is removed from all shuffle shards and so reassignment of queriers to tenants occurs.)
Relevant commits: all commits except 84b2bd4
ProtobufResponseStream.Next()sometimes returns anilmessage if the context passed toDoProtobufRequestis cancelled after at least some of the response is received from the querierThis PR fixes an issue where
Nextmight sometimes return anilmessage if the overall request context is cancelled after the query-frontend has started to receive the response from the querier and the caller has consumed all messages sent to the query-frontend. This can result in errors like those #12870 tries to mitigate.Relevant commits: 84b2bd4
Which issue(s) this PR fixes or relates to
#12551
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.