Give indexing plan a time-based ID; use it to tiebreak publish tokens#6520
Give indexing plan a time-based ID; use it to tiebreak publish tokens#6520nadav-govari wants to merge 6 commits into
Conversation
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
https://github.com/quickwit-oss/quickwit/blob/894fad03e4073585307b38d4ef40e1fa61591701/quickwit-indexing/src/source/ingest/mod.rs#L511
Here the incoming indexing_plan_id is only adopted when reset_if_needed decides to reset. For a newer plan that only adds shards to this already-running pipeline, this returns without updating self.publish_token, and the later AcquireShardsRequest is sent with the previous plan's token. If the previous owner missed the new plan, it can still reacquire the moved shard with that same old token because the metastore allows equality, so the stale-plan stealing this change is meant to prevent still happens for add-only handoffs.
https://github.com/quickwit-oss/quickwit/blob/894fad03e4073585307b38d4ef40e1fa61591701/quickwit-metastore/src/metastore/postgres/queries/shards/acquire.sql#L15
This predicate now applies to every AcquireShards caller, but queue notification sources still reacquire stale partitions in queue_sources/shared_state.rs using a pipeline-local Ulid::new() token after only checking the shard age. If the stale owner was a later-spawned pipeline, an older still-running pipeline has a lexicographically smaller token, so the grace-period reacquire returns no shard and the partition can be retried forever until some even-newer pipeline happens to take it. The monotonic plan-token check needs to be limited to control-plane-assigned ingest shards or queue stale reacquisition needs a separate path.
https://github.com/quickwit-oss/quickwit/blob/894fad03e4073585307b38d4ef40e1fa61591701/quickwit-control-plane/src/indexing_scheduler/mod.rs#L421
Ulid::new() is time-sortable but not strictly increasing: two plan applications minted in the same millisecond get random suffixes, so the later plan can compare lower than the earlier token. Since shard acquisition now uses a string >= check, that newer plan would be refused for shards already acquired by the previous plan, leaving the stale assignment in place; use a monotonic ULID generator or another strictly increasing control-plane sequence for this token.
https://github.com/quickwit-oss/quickwit/blob/894fad03e4073585307b38d4ef40e1fa61591701/quickwit-indexing/src/source/ingest/mod.rs#L438
In a rolling upgrade where a new indexer still talks to an older control plane, proto3 leaves the new indexing_plan_id field as "", and this line installs that empty string as the publish token for every ingest pipeline. The acquire query also treats recorded empty tokens as free, so multiple pipelines can reacquire and publish with the same empty token, defeating the ownership guard and allowing the stale-plan race to reappear until the control plane is upgraded; keep the old per-client token behavior or reject/defer assignments when the plan id is missing.
https://github.com/quickwit-oss/quickwit/blob/894fad03e4073585307b38d4ef40e1fa61591701/quickwit-metastore/src/metastore/postgres/queries/shards/acquire.sql#L15
During a rolling upgrade with a new metastore/control plane but some pre-change indexers, those old indexers still present legacy publish tokens containing /. Once a shard has been held by a new plan-id token, this condition rejects any legacy token regardless of the current assignment, so a shard rebalanced back onto an old indexer is reported as assigned but is never acquired or indexed until the old indexer is upgraded or a newer-token owner takes it.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78374bb73b
ℹ️ 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".
nadav-govari
force-pushed
the
nadav/indexing-plan-id
branch
from
78374bb to
23b703f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23b703f050
ℹ️ 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".
| OR publish_token IS NULL -- never acquired: free to take | ||
| OR publish_token = '' -- empty placeholder: free to take | ||
| OR publish_token LIKE '%/%' -- recorded token is legacy: superseded by any ULID | ||
| OR $4 >= publish_token -- both are ULIDs: take only if ours is newer-or-equal |
There was a problem hiding this comment.
This orders every AcquireShards token, but AcquireShards is also used by queue sources: QueueCoordinator::new still mints per-indexer Ulid::new() tokens locally, and QueueSharedState::acquire_partitions reclaims stale partitions through acquire_shards. With two indexers whose clocks are skewed, or any restarted queue source that produces a lexicographically lower ULID than the token recorded on the shard, the grace-period reacquire path returns no shard, so the message stays unprocessed until a future token happens to sort higher. The monotonic predicate needs to be limited to control-plane plan tokens, or queue tokens need to stay in the legacy/overwrite path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71097185b7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62516e198d
ℹ️ 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".
| OR publish_token IS NULL -- never acquired: free to take | ||
| OR publish_token = '' -- empty placeholder: free to take | ||
| OR publish_token LIKE '%/%' -- recorded token is legacy: superseded by any ULID | ||
| OR $4 >= publish_token -- both are ULIDs: take only if ours is newer-or-equal |
There was a problem hiding this comment.
On PostgreSQL databases whose default collation is not bytewise C, this VARCHAR comparison uses the column/database collation, but ULID time ordering is only guaranteed for ASCII/bytewise lexicographic order. In those deployments a newer plan token can compare lower than the recorded token, or a stale token can compare higher, causing AcquireShards to reject the current owner or let an older plan steal the shard; the SQL should force COLLATE "C" (or compare a decoded monotonic value) for the token ordering.
Useful? React with 👍 / 👎.
1 participant
Description
An observed bug was that if two indexers have two different indexing plans (one was missed, indexing plan broadcasting bug, etc) they both think they're the rightful order of the shard, and will re-acquire it consistently.
However, since the control plane is 1. a singleton and 2. the assigner of shards, there can only be one leader of a shard per indexing plan. And the most recent indexing plan is the source of truth.
To that end, we can assign IDs to the indexing plan such that the indexing plan ID becomes the publish token for a shard, and then each indexer acquiring a shard must supply a newer indexing plan ID (publish token) than currently exists in the metastore in order to acquire the shard. This prevents indexers with stale indexing plans from stealing the shard from indexers with the up to date one.
How was this PR tested?
Unit tests. Verified the scenario in a running cluster.