feat(cubestore): Support QUEUE ADD_AND_RETRIEVE command - #11590
Conversation
Taking a job into work costs two round-trips today: QUEUE ADD inserts the
item as pending, then QUEUE RETRIEVE flips it to active and returns the
payload. The gap between the two calls is also where another node can take
the slot.
QUEUE ADD_AND_RETRIEVE inserts and claims the item in the same atomic
RocksDB batch when the prefix has spare concurrency:
QUEUE ADD_AND_RETRIEVE [EXCLUSIVE] [PRIORITY n] [ORPHANED n]
[EXTERNAL_ID 's'] <key> '<value>' <concurrency>
Concurrency is a required argument and uses the same budget as
QUEUE RETRIEVE CONCURRENCY: prefix scoped, exclusivity and priority blind.
A brand new item is inserted with the active status right away, which
avoids an update of the just written row and its secondary indexes inside
the same batch.
It's a separate handler end to end (own AST variant, own CacheStore method,
own payload/response types), QUEUE ADD is untouched. The response extends
the QUEUE ADD one, `payload` is NULL when the item was not claimed:
| # | column | notes |
|---|---------|------------------------------------------------------|
| 0 | id | |
| 1 | added | false when the path already existed |
| 2 | pending | after the operation |
| 3 | active | active keys of the prefix, NULL when empty |
| 4 | payload | the claimed value, NULL when it was not claimed |
| 5 | extra | NULL for a freshly added item |
The claim itself (exclusivity check, heartbeat, missing payload handling)
and the concurrency budget read are extracted from queue_retrieve_by_path
into try_claim_queue_item / queue_prefix_counters, so both commands cannot
drift apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
cube store
Removes the comments that narrated the adjacent code instead of explaining it: the test scenario labels which restate the query and the expected row, the "same budget as QUEUE RETRIEVE CONCURRENCY" note which was already on the concurrency field and in queue_prefix_counters, and the two doc comments which opened by paraphrasing their own signature. The remaining comments carry a reason that is not readable from the code: the heartbeat requirement for orphaned filtering, why a new item is inserted as active right away, why the value is taken back from the inserted row instead of cloned, and why the exclusive flag cannot block an insert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #11590 +/- ##
===========================================
- Coverage 84.05% 59.28% -24.78%
===========================================
Files 257 228 -29
Lines 81582 18200 -63382
Branches 0 3681 +3681
===========================================
- Hits 68576 10790 -57786
+ Misses 13006 6861 -6145
- Partials 0 549 +549
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The sequence diagrams mixed `activate` with `-->>+` response arrows, so the lifelines were activated on the wrong participant and never deactivated. The response types were stale too: `pending` was typed as a string, `QUEUE RESULT` was missing the `id` / `external_id` columns, and `getQueryStageState` had a `TODO` instead of its response. Splits the one overloaded diagram into enqueue-and-wait, reconcile and background execution, and documents the decisions each step makes: the getResult short circuit, `added=false` when two requests share one execution, the toProcessLimit computation, the four-way admission guard in processQuery and the reconcile which refills a freed slot after ACK. Adds the fast track section for QUEUE ADD_AND_RETRIEVE: an insert which also claims the item when the prefix has a free slot, so the enqueueing request can execute without a second round trip, and cannot lose the slot in between. | Step | Normal | Fast track | |-------------------------------------|---------------|------------| | QUEUE ADD | 1 round-trip | folded | | QUEUE ACTIVE + PENDING (reconcile) | 2 round-trips | skipped | | QUEUE RETRIEVE | 1 round-trip | folded | | Window to lose the slot | ADD..RETRIEVE | none | Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 5m 11s —— View job Review: solid, well-factored feature — 2 medium, 3 low, no blockersFull review (checklist, findings, verification notes)
What's good
Findings
Suggested extra testNothing covers the documented reclaim path — a fast-tracked item whose heartbeat stops being picked up by Not verifiedI reviewed statically and did not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| key: self.parse_identifier()?, | ||
| value: self.parse_literal_string()?, | ||
| external_id, | ||
| concurrency: self.parse_integer("concurrency", false)?, |
There was a problem hiding this comment.
Grammar asymmetry with QUEUE RETRIEVE. RETRIEVE spells the budget as an optional keyword argument that defaults to 1 (sql/parser.rs:798-802):
let concurrency = if self.parse_custom_token(&"concurrency") {
self.parse_integer("concurrency", false)?
} else { 1 };Here it is a mandatory bare trailing integer after the value, so the same concept has two different surface syntaxes (QUEUE RETRIEVE CONCURRENCY 4 'p' vs QUEUE ADD_AND_RETRIEVE 'p' 'v' 4). Two downsides:
- Callers that already build
CONCURRENCY nstrings can't reuse that code path, and a bare trailing int reads ambiguously next toPRIORITY n/ORPHANED n. - It permanently closes the door on any further trailing positional argument — a future
QUEUE ADD_AND_RETRIEVE ... <value> <concurrency> <something>becomes impossible to disambiguate, whereas prefix options are freely extensible.
Consider accepting CONCURRENCY n (optionally still allowing the bare form for the driver you plan to write) so the two commands stay symmetric.
| QueueRetrieveResponse::LockFailed { pending, active } | ||
| | QueueRetrieveResponse::NotEnoughConcurrency { pending, active } | ||
| | QueueRetrieveResponse::NotFound { pending, active } | ||
| | QueueRetrieveResponse::ExclusiveAccessFailed { pending, active } => { | ||
| (None, None, pending, active) | ||
| } | ||
| }; | ||
|
|
||
| Self { | ||
| id, | ||
| // An existing item is never added twice, it's unique by path | ||
| added: false, |
There was a problem hiding this comment.
NotFound breaks the documented "the item is enqueued either way" contract.
try_claim_queue_item returns NotFound in one case: the queue item exists but its payload row is missing, in which case it deletes the queue row (cache_rocksstore.rs:790-800). Folding that into (None, None, pending, active) with added: false produces a response that says "the item was already in the queue, it just wasn't claimed" — but the row is gone by the time the batch commits.
Failure scenario: payload row for prefix:path is missing (corruption / partial eviction) → QUEUE ADD_AND_RETRIEVE 'prefix:path' 'v' 4 deletes the item, returns added = false, payload = NULL, id = <deleted id>. Per DEVELOPMENT.md:241-243 the caller then "falls back to the normal path with nothing lost, because the item is enqueued either way" — so it goes on to QUEUE RESULT_BLOCKING <deleted id> and blocks until timeout for a job nobody will ever run.
Since this command owns both the insert and the claim, it can recover instead of just reporting: when the claim lands on NotFound, the queue row was deleted, so the insert path can be re-run to write a fresh item + payload in the same batch (and report added: true). At minimum, don't report added: false for a row that no longer exists.
| // A brand new item is inserted with the active status right away, it saves | ||
| // an update of the just written row (and its secondary indexes) inside the | ||
| // same batch. The exclusive flag can never block it: the process_id of a new | ||
| // item is the process_id of the caller, see sql/cachestore.rs. | ||
| let mut item = QueueItem::new( | ||
| payload.path, | ||
| if claim { | ||
| QueueItemStatus::Active | ||
| } else { | ||
| QueueItem::status_default() | ||
| }, | ||
| payload.priority, | ||
| payload.orphaned.clone(), | ||
| payload.process_id, | ||
| payload.exclusive, | ||
| payload.external_id, | ||
| ); | ||
| if claim { | ||
| // It's important to insert heartbeat, because | ||
| // without that created datetime will be used for orphaned filtering | ||
| item.update_heartbeat(); |
There was a problem hiding this comment.
The invariant this comment relies on lives in another module and isn't enforced here.
The exclusive flag can never block it: the process_id of a new item is the process_id of the caller, see
sql/cachestore.rs.
That's true for the SQL path only, because sql/cachestore.rs:391-395 rejects EXCLUSIVE without x-process-id. queue_add_and_retrieve is a public CacheStore trait method, so any non-SQL caller can pass exclusive: true, process_id: None and this branch will happily insert an exclusive item with process_id = None directly in the Active status — precisely the shape try_claim_queue_item treats as corrupt (cache_rocksstore.rs:764-771, "Incorrect queue_item with exclusive flag, empty process_id").
It's cheap to make it locally true instead of documented: add the check next to the existing length validations at the top of the method (which already duplicate queue_add's), e.g.
if payload.exclusive && payload.process_id.is_none() {
return Err(CubeError::user(
"An exclusive queue item requires a process_id".to_string(),
));
}Then the comment can state the invariant rather than delegate it.
Minor, same block: payload.orphaned.clone() — Option<u32> is Copy, the .clone() is noise (inherited from queue_add).
| &self, | ||
| payload: QueueAddAndRetrievePayload, | ||
| ) -> Result<QueueAddAndRetrieveResponse, CubeError> { | ||
| if let Some(ref id) = payload.process_id { | ||
| if id.len() > QUEUE_ITEM_PROCESS_ID_MAX_LEN { | ||
| return Err(CubeError::user(format!( | ||
| "process_id exceeds maximum allowed length of {} characters", | ||
| QUEUE_ITEM_PROCESS_ID_MAX_LEN | ||
| ))); | ||
| } | ||
| } | ||
| if let Some(ref id) = payload.external_id { | ||
| if id.len() > QUEUE_ITEM_EXTERNAL_ID_MAX_LEN { | ||
| return Err(CubeError::user(format!( | ||
| "external_id exceeds maximum allowed length of {} characters", | ||
| QUEUE_ITEM_EXTERNAL_ID_MAX_LEN | ||
| ))); | ||
| } |
There was a problem hiding this comment.
These 18 lines are a verbatim copy of queue_add's validation (cache_rocksstore.rs:1376-1391). Given the PR's stated goal of keeping RETRIEVE and ADD_AND_RETRIEVE from drifting apart (nicely done for the claim logic via try_claim_queue_item), the same argument applies to ADD / ADD_AND_RETRIEVE validation — a small free function is enough:
fn validate_queue_ids(process_id: &Option<String>, external_id: &Option<String>) -> Result<(), CubeError>Otherwise a future change to the QUEUE_ITEM_*_MAX_LEN handling has to be made in two places, and only one of them has tests (test_queue_add_*_max_length-style coverage exists for queue_add at cache_rocksstore.rs:2795/2816, but not for the new method).
| match (id_row.get_row().get_process_id(), caller_process_id) { | ||
| (Some(_), None) => { | ||
| return Err(CubeError::user( | ||
| "Claiming an exclusive queue item requires a process_id in the connection context (x-process-id header)".to_string(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
Nit: this is a user-visible error string change for the pre-existing QUEUE RETRIEVE command — it used to be "QUEUE RETRIEVE requires a process_id in the connection context (x-process-id header)". The new wording is arguably better (it names the actual cause), and I found no assertion on the old text in rust/ or packages/, so nothing breaks — just worth calling out in the changelog since operators may be grepping logs for it.
Priority ordering is enforced by the selection step, not by the claim: QUEUE PENDING returns items highest priority first and reconcile takes toProcessLimit off the top of that list. QUEUE RETRIEVE is priority blind and is safe only because the path it gets comes from that sorted list. ADD_AND_RETRIEVE selects itself, so it had nothing to compensate for being priority blind. It now claims the item only while there is almost nothing to jump over: | condition | meaning | |------------------------------------|------------------------------------| | active < concurrency | a slot is free, as before | | pending * 2 < concurrency | the backlog is shallow | A claimed item goes straight to active and never becomes pending, so a burst onto an idle queue still fast tracks every query. Items start to accumulate in pending only when the concurrency budget is exhausted, which is exactly when the fast track should step aside and let reconcile pick by priority. The condition is written as a multiplication, because concurrency / 2 truncates to 0 for a queue with concurrency 1 (the pre-aggregation build queue), which would disable the fast track there entirely. An item which is already pending is not counted as its own backlog, otherwise claiming an existing item would never happen for the small concurrency queues. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 2m 42s —— View job Re-review of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Medium | cache_rocksstore.rs:1482-1486 |
The two clauses are independent, so together they bound nothing: at concurrency=10, active=9, backlog=4 the new item still takes the last slot ahead of 4 higher-priority items, while at concurrency=4, active=0, backlog=2 the fast track is refused although claiming would leave 3 free slots and delay nobody. active + backlog < concurrency gives zero inversion, permits strictly more harmless fast-tracking, and removes the need for the * 2 truncation workaround. |
| 2 | Low | cache_rocksstore.rs:873-874 |
QueueAddAndRetrievePayload.concurrency doc went stale in this commit — still says the claim depends only on active slots and is "the same budget as QUEUE RETRIEVE CONCURRENCY uses", which is now false in both halves. Plus DEVELOPMENT.md:243/:303 write pending < concurrency / 2, which reads as "never" under integer division at concurrency = 1. |
Prior findings — all 5 still apply, none reposted
Re-affirmed against the current tree; the underlying code is untouched by 2f53dab, so the threads stay open without a "still applies" reply:
parser.rs:711— mandatory bare trailing<concurrency>vsRETRIEVE's optionalCONCURRENCY n.cache_rocksstore.rs:911—NotFoundfolded intoadded: false+ the id of a just-deleted row, breaking the "enqueued either way" contract.cache_rocksstore.rs:1529—exclusive ⇒ process_id.is_some()invariant only enforced insql/cachestore.rs, while the trait method is public and inserts straight intoActive.cache_rocksstore.rs:1463— validation copy-pasted fromqueue_add, only one copy tested.cache_rocksstore.rs:762— user-visibleQUEUE RETRIEVEerror string change, worth a changelog line.
Nothing was resolved — I only resolve my own threads whose concern has gone away, and none has.
Not verified
Static review again: no cargo build / cargo test / cargo clippy (too expensive for this job), and origin/master has no merge base in this shallow checkout, so I diffed the four PR commits directly. The failure scenarios in finding #1 are hand-evaluated against the condition at cache_rocksstore.rs:1486, not observed at runtime.
| // A deep backlog is left to QUEUE PENDING + reconcile, which pick by priority. | ||
| // Claiming the item being added ignores priority, it's acceptable only while | ||
| // there is almost nothing to jump over. | ||
| let claim = active.len() < (payload.concurrency as usize) | ||
| && backlog * 2 < (payload.concurrency as u64); |
There was a problem hiding this comment.
The two conditions don't compose into "no item is jumped over" — they're both too strict and too loose.
The stated goal is to fast-track only when there is almost nothing to jump over. But the two clauses are independent, so neither implies that:
Too loose — inversion still happens when the queue is nearly saturated. concurrency = 10, active = 9, backlog = 4 (all PRIORITY 100): 9 < 10 ✓ and 4 * 2 = 8 < 10 ✓ → the new PRIORITY 0 item takes the last free slot and 4 higher-priority pending items keep waiting. That's exactly the inversion the commit sets out to bound, and the bound scales with concurrency, so a big queue permits jumping over up to concurrency/2 - 1 items.
Too strict — the fast track is refused when it provably harms nobody. This is what the new test at cache_rocksstore.rs:2985 encodes: concurrency = 4, active = 0, backlog = 2 → refused, even though claiming would leave 3 free slots for the 2 pending items. Nobody is delayed by a single microsecond. This case is not hypothetical during the rollout described in the > **Status:** note — while other nodes still emit QUEUE ADD, pending fills up while slots sit idle, and every fast-track attempt is refused.
One condition covers both directions — claim only when there is a slot for this item and for every item already queued:
// Claiming the item being added ignores priority. It's harmless exactly while a slot
// is left over for every pending item too, so nothing can be jumped over; a deeper
// backlog is left to QUEUE PENDING + reconcile, which pick by priority.
let claim = (active.len() as u64) + backlog < (payload.concurrency as u64);Post-claim free slots are concurrency - active - 1 >= backlog, so every pending item still has a slot: the inversion is zero, not bounded. It also keeps the two properties the commit message cares about — concurrency = 1 still fast-tracks an idle queue (0 + 0 < 1), and a burst onto an idle queue still claims every item until the budget is gone — while dropping the need for the * 2 truncation workaround entirely.
The test_queue_add_and_retrieve_backlog expectations would flip for the first case, which is the point: that case should claim.
Covers the backlog condition end to end, the store level test alone did not
exercise the parser and the SQL layer. The declining case is the valuable one:
every concurrency slot is free, so the item would have been claimed without the
condition.
QUEUE ADD "STANDALONE#queue:1" / :2 -- 2 pending, 0 active
QUEUE ADD_AND_RETRIEVE "STANDALONE#queue:3" "payload3" 4 -- 2*2 < 4 false, payload NULL
QUEUE ADD_AND_RETRIEVE "STANDALONE#queue:4" "payload4" 7 -- 3*2 < 7 true, claimed
Replacing the condition with `true` fails both this test and the store level
one, so neither of them passes vacuously.
queue_add_and_retrieve_row accepts an optional active, it could not express the
NULL which a declined claim returns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 2m 26s —— View job Re-review of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | cubestore-sql-tests/src/tests.rs:12199 |
Numeric path suffixes make key == id for every item, so the QUEUE ACTIVE assertion can't distinguish the key and id columns (a swap in the SQL layer still passes), and neither can queue_add_and_retrieve_row("4", …, Some("4"), …). queue_retrieve_extended avoids this with a non-numeric key. Plus assert_queue_add_and_retrieve_columns is called on the declined response but not the claimed one. |
Prior findings — all 7 still apply, none reposted
b62a000 touches no production code, so every prior thread stands unchanged. Nothing resolved; I only resolve my own threads whose concern has gone away.
parser.rs:711— mandatory bare trailing<concurrency>vsRETRIEVE's optionalCONCURRENCY n.cache_rocksstore.rs:1486— the two claim clauses bound nothing: too loose near saturation, too strict on an idle queue. This commit is the one place worth flagging the interaction: the new test's case 1 (concurrency = 4,active = 0,backlog = 2→ refused, with 3 slots that would sit idle) now encodes the too-strict half at the SQL level as well as the store level. If you adoptactive + backlog < concurrency, both tests flip for that case — which is the intended outcome, not a regression. Worth deciding the condition before locking it into two test layers.cache_rocksstore.rs:911—NotFoundfolded intoadded: false+ the id of a just-deleted row.cache_rocksstore.rs:1529—exclusive ⇒ process_id.is_some()enforced only insql/cachestore.rswhile the trait method inserts straight intoActive.cache_rocksstore.rs:874— staleconcurrencydoc +pending < concurrency / 2integer-division trap inDEVELOPMENT.md:243/:303.cache_rocksstore.rs:1463— validation copy-pasted fromqueue_add, only one copy tested.cache_rocksstore.rs:762— user-visibleQUEUE RETRIEVEerror string change, worth a changelog line.
Not verified
Static review again: no cargo build / cargo test / cargo clippy (Rust build for this workspace is too expensive for this job). origin/master has no merge base in this shallow checkout, so I diffed the PR commits directly. The claim-decision arithmetic above is hand-evaluated against cache_rocksstore.rs:1483-1486 and active_keys_to_value, not observed at runtime — including the "replace the guard with true and it fails" check.
| &vec![Row::new(vec![ | ||
| TableValue::String("4".to_string()), | ||
| TableValue::String("4".to_string()), | ||
| TableValue::String("active".to_string()), | ||
| TableValue::Null, | ||
| ]),] |
There was a problem hiding this comment.
Nit, test strength: the numeric path suffixes make key == id for every item, so this assertion can't tell the key and id columns apart. QUEUE ACTIVE returns (key, id, status, extra) — swap those two columns in the SQL layer and this test still passes, and so does queue_add_and_retrieve_row("4", true, 3, Some("4"), ...) above (id and active are also both "4").
The pre-existing queue_retrieve_extended test avoids this by using a non-numeric key (tests.rs:11544-11546: key "queue_key_3", id "3"). Naming the paths STANDALONE#queue:key1 … :key4 here costs nothing and makes the columns distinguishable — worth it since ADD_AND_RETRIEVE is the command that newly reports active alongside id in one row.
Also minor: assert_queue_add_and_retrieve_columns is called on the first response but not on the second (tests.rs:12176). Since the second is the claimed case — the one whose column set the driver will actually read — it's the more useful of the two to pin.
The doc comments on QueueCommand::AddAndRetrieve and on the concurrency field of QueueAddAndRetrievePayload described the claim rule as "less than `concurrency` active items", which stopped being true when the backlog condition was added. Both of them now point at the budget instead of spelling out the rule, which lives in queue_add_and_retrieve alone, so there is nothing left to drift. Drops the arithmetic narration from the backlog tests as well: the numbers are in the call two lines below, what a reader cannot see is that the declining case would have been claimed before the condition existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 1m 23s —— View job Re-review of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | packages/cubejs-query-orchestrator/DEVELOPMENT.md:245 |
The prose half of the staleness note is untouched: docs say pending < concurrency / 2 (:245, :303), code says backlog * 2 < concurrency. Under integer u64 division concurrency = 1 reads as "never fast-track" — the exact case the * 2 form exists to preserve. Posted with a suggestion; also notes that /// The same budget as QUEUE RETRIEVE CONCURRENCY uses is now true of the number but not of the decision. |
Thread bookkeeping
- Resolved
PRRT_kwDOCOH19M6ahOKN(staleconcurrencydoc comment) — the line it was anchored to is gone and the concern is addressed. The DEVELOPMENT.md half it also carried is reposted as the finding above, on the file that still has it, so nothing is dropped by resolving. - 7 prior threads re-affirmed, none reposted (production code untouched by
fe6380e):parser.rs:711— mandatory bare trailing<concurrency>vsRETRIEVE's optionalCONCURRENCY n.cache_rocksstore.rs:1485— the two claim clauses bound nothing: too loose near saturation (concurrency=10, active=9, backlog=4still inverts), too strict on an idle queue (concurrency=4, active=0, backlog=2refused with 3 slots idle).active + backlog < concurrencygives zero inversion and drops the* 2. Still the one finding worth deciding before merge — it is now encoded in two test layers.cache_rocksstore.rs:911—NotFoundfolded intoadded: false+ the id of a just-deleted row.cache_rocksstore.rs:1529—exclusive ⇒ process_id.is_some()enforced only insql/cachestore.rswhile the trait method inserts straight intoActive.cache_rocksstore.rs:1463— validation copy-pasted fromqueue_add, only one copy tested.cache_rocksstore.rs:762— user-visibleQUEUE RETRIEVEerror string change, worth a changelog line.tests.rs:12202— numeric path suffixes makekey == id, so theQUEUE ACTIVEassertion can't tell the two columns apart.
Not verified
Static review again: no cargo build / cargo test / cargo clippy (Rust build for this workspace is too expensive for this job). origin/master has no merge base in this shallow checkout, so I diffed the PR commits directly. The integer-division claim above is hand-evaluated against cache_rocksstore.rs:1485, not observed at runtime.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
Claude finished @ovr's task in 59s —— View job Re-review of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | packages/cubejs-query-orchestrator/DEVELOPMENT.md:303 |
Only one of the two occurrences was fixed. :303 still restates the rule as pending < concurrency / 2, so the file now contradicts itself two paragraphs apart — arguably worse than one consistently-wrong statement, since a reader landing on the prose has no reason to scroll back to the bullet. Same integer-division trap: at concurrency = 1 the / 2 form reads "never fast-track", contradicting the very next sentence about bursts onto an idle queue. Posted with a suggestion, plus a note that the mermaid label at :266 (claim if active < concurrency) is still the pre-2f53dab rule. |
Thread bookkeeping
- Resolved: none this pass — the DEVELOPMENT.md thread that carried this concern was already resolved when the suggestion was committed, which is why the residual at
:303is posted as a fresh comment on the line that still has it rather than as a reply. - 7 prior threads re-affirmed, none reposted (no production code touched by
ab917ac):parser.rs:711— mandatory bare trailing<concurrency>vsRETRIEVE's optionalCONCURRENCY n.cache_rocksstore.rs:1485— the two claim clauses bound nothing: too loose near saturation (concurrency=10, active=9, backlog=4still inverts), too strict on an idle queue (concurrency=4, active=0, backlog=2refused with 3 slots idle).active + backlog < concurrencygives zero inversion and drops the* 2. Still the one thing worth deciding before merge — and note that this docs churn is a symptom of it: the* 2form is what keeps needing careful prose. The single-clause version needs no explanation of integer truncation at all.cache_rocksstore.rs:911—NotFoundfolded intoadded: false+ the id of a just-deleted row.cache_rocksstore.rs:1529—exclusive ⇒ process_id.is_some()enforced only insql/cachestore.rswhile the trait method inserts straight intoActive.cache_rocksstore.rs:1463— validation copy-pasted fromqueue_add, only one copy tested.cache_rocksstore.rs:762— user-visibleQUEUE RETRIEVEerror string change, worth a changelog line.tests.rs:12202— numeric path suffixes makekey == id, so theQUEUE ACTIVEassertion can't tell the two columns apart.
Not verified
Static review again: no cargo build / cargo test / cargo clippy (Rust build for this workspace is too expensive for this job). origin/master has no merge base in this shallow checkout, so I inspected ab917ac directly and grepped the current tree for both spellings of the condition. The integer-division claim is hand-evaluated against cache_rocksstore.rs:1485, not observed at runtime.
| — it is safe only because the path it is given came from that sorted list. | ||
|
|
||
| The fast track selects itself, so it is priority blind with nothing to compensate. That is | ||
| what the `pending < concurrency / 2` condition bounds: with a shallow backlog there is |
There was a problem hiding this comment.
ab917ac fixed the bullet at :245, but the prose restates the same condition here in the old integer-division form — so the file now contradicts itself two paragraphs apart, which is worse than the original single wrong statement: a reader who lands here has no reason to scroll up.
Same argument as before: at concurrency = 1 the / 2 form reads as pending < 0 → "never fast-track", while the code (cache_rocksstore.rs:1485, backlog * 2 < concurrency) claims on an idle queue (0 * 2 < 1 ✓) — the case the very next sentence relies on ("a burst onto an idle queue still fast-tracks every query").
| what the `pending < concurrency / 2` condition bounds: with a shallow backlog there is | |
| what the `pending * 2 < concurrency` condition bounds: with a shallow backlog there is |
Related, same section: the mermaid note at :266 says insert, then claim if active < concurrency, which is the pre-2f53dab rule. Since the diagram is the part most people read, it is worth either adding the backlog clause or making it point at the bullets — e.g. One atomic batch:<br/>insert, then claim if the prefix allows it.
1 participant
Taking a job into work costs two round-trips today: QUEUE ADD inserts the item as pending, then QUEUE RETRIEVE flips it to active and returns the payload. The gap between the two calls is also where another node can take the slot.
QUEUE ADD_AND_RETRIEVE inserts and claims the item in the same atomic RocksDB batch when the prefix has spare concurrency:
refs #8490