Use generation ID for routing table entries, and react to the ingester pool - #6791
Conversation
nadav-govari
changed the title
nadav-govari
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fae0c7f35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .sum(); | ||
|
|
||
| for (node_id, open_shard_count) in per_ingester_count { | ||
| let Some(generation_id) = ingester_pool.get(&node_id).map(|entry| entry.generation_id) |
There was a problem hiding this comment.
When GetOrCreateOpenShards computes a response for generation A but the ingester restarts before the router merges that response, the pool can already contain generation B; this lookup then labels A's shard assignments as belonging to B. For a newly inserted routing node, the default positive capacity makes B immediately eligible even if it has none of those shards, and because B never broadcasts updates for a source it does not host and later CP merges do not remove absent nodes, this inaccurate route can persist and recreate the NoShardsAvailable failures this change is intended to prevent. The generation must be supplied by the authoritative response or the merge must detect intervening membership churn and retry rather than inferring it from the current pool.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the exact TODO that comes a line later. This is its own race condition. Unfortunately the fix for this is sending generation ID from the control plane, which becomes much more complicated, because it uses the shard proto which is also in the metastore model. I'll re-explore how invasive that is but when I looked into it it seemed like a mess.
| else { | ||
| // TODO: decide what you want to do here exactly. This might be important | ||
| continue; |
There was a problem hiding this comment.
When the control plane returns an open shard before this router's independent cluster-change listener has inserted that ingester into the local pool, this branch silently drops the authoritative assignment and the method still marks the routing entry as seeded from the control plane. The current request then burns through its five retries with NoShardsAvailable if the pool update is delayed, whereas previously the assignment remained in the table and became usable as soon as the ingester appeared. Retain an unresolved assignment for later generation reconciliation, or leave the entry explicitly unseeded and wait/retry rather than treating this merge as successful.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| .unwrap_or(false); | ||
|
|
||
| if !is_ready { | ||
| .filter(|ingester| ingester.status.is_ready()) |
There was a problem hiding this comment.
super nitpick: I'd put that in a separate if statement
let Some(ingester) = ... else { return false; }
ingester.generation_id != self.generation_id
&& ingest.is_ready()
There was a problem hiding this comment.
This changes the logic unfortunately- if it's not ready, we need to add it to unavailable ingesters.
6fae0c7 to
89181a2
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89181a2250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let persist_summary = PersistRequestSummary { | ||
| ingester_id: ingester_id.clone(), | ||
| generation_id: pool_entry.generation_id, | ||
| subrequest_ids, |
There was a problem hiding this comment.
When an ingester restarts after pick_node validates generation A but before this second pool lookup, pool_entry belongs to generation B, so the router sends subrequests chosen from A's routing entry to B and records B in the summary. If B does not host that source, this produces the same NoShardsAvailable failure the generation check is intended to prevent. Carry the selected node's generation into per_ingester_persist_subrequests and require it to match this pool entry before dispatching.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
2 participants
Description
Builds on #6680.
Currently, we rely on the ingester pool to gate ingester availability in the routing table. We never remove entries. As a result, we don't react when ingesters leave and rejoin the cluster. This mostly resolves itself on one-index clusters, because shards are more or less interchangeable. However, there's a logical problem with this on multi-index clusters: an ingester leaves the cluster, and rejoins, but might not get assigned any shards for a source it had previously, in which case it becomes a valid, inaccurate candidate for persist requests for that source.
In my testing with 9 source of varying size from "huge" to "very tiny", I observed a persistent 1% ingest error rate, that upon further investigation was due to this bug- requests were arriving at ingesters that simply didn't host any shards for that source, because they had done so in a previous incarnation.
The fix is twofold:
How was this PR tested?
Unit testing. Live cluster testing incoming.