Repository navigation
perf(indexer): sparse binary + zstd stats sketches; 15-minute GC default - #2020
Conversation
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz generally very on board with this. My only headline note/question is about the writer-side ceiling/bound in sketch_cas.rs:190. The reader bound makes sense to me. I'm a bit nervous that the writer bound risks turning a "this ledger's sketch got big" situation into a "this ledger can never index again". The reader, for sure, needs a ceiling and the ceiling needs the writer to never produce something the reader rejects. The PR gives evidence for an alternative (the reseed path can decline into a "skip the sketch with an error log" option), and if upload_stats_sketch were to follow that pattern, at least the index/reindex wouldn't be permanently bricked for that ledger. More details in full review below:
This is a really nice format change: sparse-or-dense per HLL, one zstd frame, a first-byte sniff so v1 keeps reading, and no root change, so the upgrade is just "the next build reads v1 and writes v2". The decoder is carefully built. decompress refuses an over-ceiling header before allocating anything, pins the frame to the slice and the output to the declared length, the register index is a u8 so a sparse pair can't reach past 256 registers, and MAX_RANK = 57 is exactly the largest rank insert_hash can produce. Splitting Unsupported from Malformed, so a future format takes the full-rebuild fallback instead of silently reseeding, is the right call too.
The one I'd most like to see folded in:
- 🟠
fluree-db-indexer/src/stats/sketch_cas.rs:190— the 256 MB ceiling is also enforced on the write side, and both writers propagate it with?, so a ledger whose sketch crosses it fails the incremental build and then the full-rebuild fallback, the worker retries both every ~30 s forever,fluree reindexfails the same way, and eventually backpressure rejects transactions (I reproduced it with the ceiling lowered to 64 bytes). That's roughly 500k dense or ~4M sparse entries. Havingupload_stats_sketchskip the sketch with an error log (the reseed path you already accept) instead of failing the build would fix it; details and a snippet inline.
The rest:
- 🟡 Four places still say the GC default is 30, including the
--gc-min-time-minshelp text (fluree-db-server/src/config.rs:492) anddocs/reference/connection-config-jsonld.md:80(anchored atgc/mod.rs:178). - 🟡
docs/operations/configuration.md:506— the guard now equals the server's defaultquery_timeout_ms; one sentence saying the guard should stay above the longest query, and thatquery_timeout_ms = 0leaves nothing bounding one, would make the coupling visible. - 🟡 On the release note under "Upgrade and downgrade": a rolling upgrade reaches the downgrade path without anyone downgrading. Where the indexer runs on the raft leader, if leadership goes back to a not-yet-upgraded node after an upgraded one has indexed, that node can't read the v2 sketch, reseeds, and NDV stays thin until
fluree reindex. Your test docstring already says "downgrade or mixed-version"; the release note could say it too, with "upgrade the leader last" as the guidance.
On "Not in this PR": I agree all three are separable. Compressing the root's inline stats is a root-format change every reader has to ship before any writer, paged sketches is a big piece of work you've sized well, and the reindex cadence is a product call. The 🟠 above is what makes the gap until paged sketches safe.
Adherence to repo commitments:
- Patterns/abstractions: ✔ One blob kind and the existing
sketch_ref, sniffed by first byte; both writers shareupload_stats_sketch; #2018'sset_sketch_refretirement andall_cas_idscoverage apply to v2 unchanged, so the v1 → v2 transition retires the v1 CID like any other build. - Performance (speed first, memory second): ✔ Indexer-only, no query-path change. By your numbers each build writes ~1.4% of the bytes it used to (13 MB → 176 KB) and decodes in a tenth of the time, and decoded entries now hold 256-byte register arrays instead of two 512-character hex strings, so memory goes down too. No performance-degradation risk.
- Deployment targets: ✔
fluree-db-indexeris a non-wasm dependency offluree-db-api,zstdwas already a dependency of the indexer and already in the wasm build throughfluree-db-core, andwasm32/wasm-smokepassed on this head. The incremental indexer is the sketch's only reader, so on solo's Lambdas onlyIndexingLambdasees it, and the mixed-version case is the raft-leader one above. Solo's standalone inherits the 15-minute guard throughIndexerConfig::default(); its query deadline defaults to 300 s. - Testing: ✔ Pinned v1 and v2 fixtures, the crossover, truncation and every rejection case, plus incremental-vs-reindex integration tests on default and named graphs with retractions; all ran in CI by name. I mutated the tie-break, the frame-length check and the decoder's HLL order, and each went red where it should.
- Conventions: ✔ Thorough commit bodies, fmt and clippy clean, configuration docs updated; the four stale "30"s above are what's left.
Verified locally at 37a2f3ced: cargo fmt --check; clippy -D warnings on fluree-db-indexer + fluree-db-stats (all targets, all features) and on fluree-db-api's grp_index; sketch_cas (22) and it_stats_sketch_format + it_trigger_index_incremental (5), all passing; four mutations, each restored, with the suites green again afterwards.
Cross-PR note: where this lands. The test I'd use for next is whether new code runs on data the previous version persisted, without a migration. This passes: the reader still takes v1, and the next build writes v2. So by that test it can ship from main. The direction the test doesn't cover is an old indexer meeting a v2 sketch. A 4.2.0–4.2.3 indexer reseeds, and NDV reads low until a reindex. On solo's Lambda stacks an old indexer invocation can also meet one mid-deploy. I'd handle that with the downgrade note you already wrote rather than by moving the PR, but if your releasing.md rule is meant to cover any persisted-format change in either direction, it's your call. On #2017 either way: its triple-term datatype tag is a u8 like the others and its link counts live in root stats, so the v2 format needs nothing for it. On a local merge of #2017 and this branch, your sketch tests, #2018's locality tests and #2017's stats and triple-term suites all pass once kind_name stays and #2017's RootReadCounting gets an is_remote() (the note on #2018).
Approving so you can merge when ready, but maybe worth having the write-side ceiling degrade instead of fail first; the doc nits are quick to take along with it.
| write_hll(&mut payload, &e.values_hll); | ||
| write_hll(&mut payload, &e.subjects_hll); | ||
| } | ||
| if payload.len() > MAX_PAYLOAD_BYTES { |
There was a problem hiding this comment.
🟠 Should address — a sketch past the 256 MB ceiling now fails every index build of that ledger, incremental and full alike.
to_bytes returns an error once the payload passes MAX_PAYLOAD_BYTES, and both writers propagate it: upload_stats_sketch (build/upload.rs:48) is awaited with ? from the incremental build (incremental.rs:2280) and from the full rebuild (rebuild.rs:887). So the incremental build fails, build_index_for_ledger falls back to a full rebuild (lib.rs:227-245), and the rebuild fails on the same sketch.
The background worker treats that as an ordinary failure and retries it (on_build_error → schedule_retry, orchestrator.rs:2373-2437, backoff capped at 30 s), so it repeats an incremental attempt plus a full rebuild every ~30 s indefinitely, and a trigger_index without a timeout never returns. Each incremental attempt also uploads its new leaves and branches in Phase 2 before it reaches the sketch in Phase 3b, so those accumulate in storage with no root referencing them until someone runs fluree sweep. fluree reindex goes through the same rebuild, so there's no operator escape, and novelty grows until reindex_max_bytes starts rejecting transactions. v1 had no ceiling: at that size it wrote a very large JSON file, but it kept indexing.
How far away is that? A dense entry is about 537 payload bytes (23 fixed, 9 per datatype, two 257-byte HLLs), so roughly 500k (graph, property) entries with ~175+ distinct values each cross 256 MiB; at the sample ledger's sparse mix (742 KB over 11,488 entries, ~65 B each) it's around 4M. Both are in the "around a million entries" range the body names for paged sketches. I ran it with MAX_PAYLOAD_BYTES lowered to 64 bytes locally. incremental_build_carries_ndv_from_a_v2_sketch hung in its first trigger_index until I killed it at 15 minutes; a scratch test with a 20 s timeout got IndexTimeout(20000) with the worker Pending and last_error: "Serialization error: stats sketch payload of 798 bytes exceeds the 64-byte ceiling", and reindex returned the same error.
The ceiling makes sense on the read side as a bound on what a corrupt header can make us allocate. On the write side I think it should degrade rather than fail: if upload_stats_sketch logged at error and returned Ok(None), the build would publish without a sketch, set_sketch_ref(None) would retire the old one, and the next build would reseed from the root's stats, which is the same degradation the PR already accepts for an unreadable sketch. Illustrative:
let bytes = match HllSketchBlob::from_properties(index_t, properties).to_bytes() {
Ok(bytes) => bytes,
// Past what the format holds: index without a sketch rather than not at
// all. The next build reseeds NDV from the root's stats.
Err(e) => {
tracing::error!(error = %e, entries = properties.len(), "stats sketch not written");
return Ok(None);
}
};|
|
||
| /// Default minimum age (in minutes) before an index can be garbage collected | ||
| pub const DEFAULT_MIN_TIME_GARBAGE_MINS: u32 = 30; | ||
| pub const DEFAULT_MIN_TIME_GARBAGE_MINS: u32 = 15; |
There was a problem hiding this comment.
🟡 Nit — four places still say the default is 30.
fluree-db-server/src/config.rs:492: the doc comment on the clap field, which has nohelp =override, so it's the--gc-min-time-minshelp text: "Minimum age in minutes before an index version can be GC'd (default 30)".docs/reference/connection-config-jsonld.md:80: "defaults.indexing.gcMinTimeMins… (default: 30)".fluree-db-indexer/src/gc/mod.rs:187: "(None = default 30)" onCleanGarbageConfig::min_time_garbage_mins.fluree-db-indexer/src/gc/collector.rs:327: "(default: 30)" inclean_garbage's retention notes.
The first two are user-facing. So minor, but if you agree it's right, I'd rather see them folded in with the rest than lost in the backlog.
| | ---------------------------- | --------------------------------- | ------- | ----------------------------------------------- | | ||
| | `--gc-max-old-indexes` | `FLUREE_GC_MAX_OLD_INDEXES` | `5` | Old index versions to retain before GC | | ||
| | `--gc-min-time-mins` | `FLUREE_GC_MIN_TIME_MINS` | `30` | Minimum age (minutes) before an index version can be collected. Protects queries that started against an older version. ANDed with the count, so the slower of the two wins | | ||
| | `--gc-min-time-mins` | `FLUREE_GC_MIN_TIME_MINS` | `15` | Minimum age (minutes) before an index version can be collected. Protects queries that started against an older version. ANDed with the count, so the slower of the two wins | |
There was a problem hiding this comment.
🟡 Optional — the new default equals the server's default query_timeout_ms, and the docs don't say the two are coupled.
This is more of a docs ask than a request to change the number. DEFAULT_QUERY_TIMEOUT_MS is also 15 minutes (fluree-db-api/src/server_defaults.rs:17), and solo's Lambdas cap at 900 s, so the guard went from twice the longest default query to exactly it. And the garbage record's created_at_ms is stamped in root assembly (root_assembly.rs:121), before the root is written and published, so a query that opened the previous root in that short tail and ran to its timeout can outlive the guard by the tail. At the defaults I don't think that matters much, since that query is about to be cancelled anyway. Where it does matter is query_timeout_ms = 0, which this page documents as "disables the timeout", and an embedder that holds one snapshot across many queries: their protection halves, and "Protects queries that started against an older version" doesn't say it now depends on queries finishing inside 15 minutes.
Maybe one sentence here, something like "keep this above your longest query; query_timeout_ms defaults to 15 minutes, and with it set to 0 nothing bounds a query," would cover it. A startup warning when gc_min_time_mins * 60_000 <= query_timeout_ms would be the stronger version, the way orchestrator.rs:799-806 already derives its listing freshness from the guard. Minor and non-blocking, but if you agree, I'd rather see it in this PR than lost in the backlog.
There was a problem hiding this comment.
Addressed in bfb8382
Took the doc sentence, not the startup warning: with both defaults at 15 minutes, <= would fire on every default start, and < wouldn't cover the case you raised.
| /// Asserts the build that produced `incremental` was incremental, and that | ||
| /// it carried the prior sketch's registers: every user-graph stat, NDV | ||
| /// included, must equal a full rebuild's rather than the base-root floor. | ||
| fn assert_matches_full_rebuild( |
There was a problem hiding this comment.
👍 Comparing every user-graph stat against a full reindex, with a leaf count proving the build was incremental and a precondition that the workload moves NDV past the reseed floor, is the right way to test a reader whose failure is silent. I swapped the two HLL bindings in decode_v2 (values read into subjects_hll and vice versa, a one-line bug that leaves every count exact) and three of the four tests went red on NDV alone, e.g. (360, 336, 380, …) against the rebuild's (360, 367, 363, …).
Under a sustained publish rate the age guard, not gc_max_old_indexes, bounds retention, so halving it halves the index versions kept on disk. Explicit gc_min_time_mins settings are unaffected.
The HLL stats sketch was JSON with every register hex-encoded, rewritten whole on every index build. A many-graph ledger has thousands of (graph, property) entries whose HLLs touch a handful of registers; a sample ledger at 873 named graphs wrote a 13.0 MB sketch per build. Format v2: an uncompressed header (magic FHLL, version, precision, index_t, entry count, payload length) and one zstd frame of entries sorted by (g_id, p_id). Each HLL is sparse (index, rank) pairs or dense registers, whichever is smaller. On the sample ledger: 13.0 MB -> 176 KB, decode 25 ms -> 2.6 ms. - The reader takes v1 JSON and v2; the writer emits v2 only. - Decompression is bounded by a 256 MB ceiling and checked against the declared length; the entry count is checked before allocating. - An unsupported version is an error out of the incremental build, which routes to a full rebuild. Corrupt or unreadable sketches keep the base-root reseed, now logged at error. - Both writers share upload_stats_sketch. load_sketch_blob and the JSON writer are removed.
Retractions and named graphs through SPARQL UPDATE: counts clamp to zero, a datatype drops out, a property is fully retracted. The incremental-vs-rebuild check now compares every user-graph stat (count, NDV, last_modified_t, datatypes) rather than one property's count and NDV, which a sketch that lost its datatypes would have passed.
Commits under the default thresholds also queued background index builds, so a build's leaf-write count could include another build's writes and the incremental-path check flaked (CI: 128 vs 127 leaves). Test commits now never trigger a build. The retraction workload reaches most leaves of the graphs it touches, so it also gets a named graph the novelty never touches, which a full rebuild must rewrite and an incremental build does not.
37a2f3c to
594d82c
Compare
… build The encoder enforces the reader's 256 MB payload ceiling, and both build paths propagated the error. A ledger whose sketch crossed it failed the incremental build and the full-rebuild fallback alike, so the worker retried forever, `fluree reindex` failed the same way, and novelty grew until transactions were rejected. upload_stats_sketch now logs at error and returns None for any encode error (the ceiling, entry count, per-entry datatype count). The build publishes without a sketch, the old one is retired, and the next build reseeds NDV from the root's stats -- the same degradation already accepted for an unreadable sketch. Storage write errors still fail.
Four references still gave the old 30-minute default, including the --gc-min-time-mins help text. The configuration page now notes that the guard equals the default query timeout and that query_timeout_ms = 0 leaves nothing bounding a query.
|
Rolling-upgrade note (upgrade the leader last) added to "Upgrade and downgrade" in the description. Shipping from |
2 participants
Stacked on #2018.
#2018 stops superseded stats sketches from leaking. Each index build still rewrites the whole-ledger HLL stats sketch, and that sketch was JSON with all 256 registers of every HLL hex-encoded. A ledger with many small named graphs has thousands of (graph, property) entries whose HLLs touch a handful of registers, so the sketch grew by about 13–15 KB per named graph. The ledger behind #2018 has 873 named graphs and 11,488 entries and wrote a 13.0 MB sketch on every build. With the default of one build per commit, that is 13 MB written per commit, all of it retained for the GC age guard's 30 minutes.
1. Stats sketch format v2
An uncompressed header (magic
FHLL, format version, HLL precision,index_t, entry count, payload length), then one zstd frame (level 1) holding the entries, sorted by(g_id, p_id). Each HLL is stored sparse, as(register, rank)pairs, or dense, as 256 register bytes, whichever is smaller; ties go to dense.On that ledger's sketch (release build):
22,875 of its 22,976 HLLs are stored sparse. A synthetic sketch with every HLL dense (2,000 entries) is about 2.3 MB as v1 and 119 KB as v2, so dense-heavy ledgers do not regress.
Statistics, HLL precision, the in-memory representation and planner inputs are unchanged. The root format is unchanged:
sketch_refpoints at a v2 blob instead of a v1 one.{) and v2. The writer emits only v2.load_sketch_blob(no callers) and the JSON writer are removed.build::upload::upload_stats_sketch. A sketch the format cannot hold (past the 256 MB ceiling, for instance) is logged at error and skipped rather than failing the build; the next build reseeds from the root's stats, as for an unreadable sketch.Upgrade and downgrade
fluree sweeplike any superseded sketch.fluree reindex).fluree reindex. Upgrade the leader last.SketchDecodeError::Unsupported. The incremental build fails and takes the existing fallback to a full rebuild, which regenerates exact stats instead of reseeding.2. Default GC minimum age: 30 → 15 minutes
Under a sustained publish rate, retention is bounded by the age guard, not by
gc_max_old_indexes: a ledger indexing every few seconds keeps every version from the lastgc_min_time_mins. Halving the default halves what such a ledger keeps on disk. Explicitgc_min_time_mins/FLUREE_GC_MIN_TIME_MINSsettings are unaffected. The configuration docs are updated.Tests
sketch_cas.rs).it_stats_sketch_format.rs). The sketch has one reader, and a failed read degrades silently: counts stay exact and only NDV thins. So these tests compare every user-graph stat (count, NDV,last_modified_t, datatypes) after an incremental build against a full reindex of the same ledger, on workloads where the reseed floor and the true NDV diverge. A leaf-write count shows each build under test was incremental rather than a fallback rebuild.Not in this PR
reindex_min_bytes = 100. Whether that should become a byte floor plus a maximum-lag timer is a product decision.