fix(store): a read plan tracks its keys, not a tally (RFC 0025) - #503
Open
rmanibus wants to merge 3 commits into
Open
fix(store): a read plan tracks its keys, not a tally (RFC 0025)#503rmanibus wants to merge 3 commits into
rmanibus wants to merge 3 commits into
Conversation
packGroupPlan held a count per pack, and consume ran on every serve from a packed location — so a read nobody declared decremented a declared pack's budget. Packs mix five namespaces, so this is the ordinary case: a traversal declares a batch of filemeta refs and then reads, undeclared, the content objects those refs name, the nodes it discovers mid-walk, and whatever path resolution needs, a good share of them out of the packs it just declared against. The declared reads behind them found the budget spent and fell back to the estimate. The plan now names its keys. PlanReads already materialises every key to sort it, so the set costs a map slot apiece over the caller's own strings — bounded by the batch (8,192 refs for every streaming traversal), never by the catalog. consume spends only a declared key, and fetchWhole is asked per key rather than per pack: a pack being named by the plan says nothing about a read the plan does not name, and answering a cat of one file from someone else's eighty declared objects transfers 8 MB to return a few hundred bytes. Two consequences worth stating. Consume now runs on the miss path too, because a miss is a read: counting only cache hits left every named pack claiming one object still outstanding — the one whose miss brought the body in — so a plan never reached the zero that retires it, and a pack read entirely by ranged reads kept stating the demand it began with. And the declared set is a multiset, since a deduplicated content object is legitimately declared once per referencing file; RFC 0025 records de-duplicating those as a measured mistake. This is the precondition RFC 0025 sets on re-running the planned-versus-unplanned count in "The estimator is load-bearing": that count was taken on this accounting, so an unknown share of the unplanned misses it records are declared reads whose slot was taken.
…repository (RFC 0025) aging.sh could run restore and check. RFC 0025 counts six commands, and the four it could not run were measured by hand each time, which is why `diff` was invoked without the two snapshot IDs it requires through four rounds and reported zero. OPS gains ls, find and diff. backup and prune go in a separate FINAL_OPS, because they change the repository: a backup adds one, which is this script's independent variable, and a prune deletes snapshots and rewrites packs, so either at a checkpoint silently redefines every checkpoint after it. ATTACH=1 measures against the repository already in MinIO rather than aging a new one, and KEEP_STORE=1 leaves the container up for it. That is what makes a read-policy comparison mean anything here. Running the whole script twice compares two builds against two repositories, and pack composition is not deterministic — PackStore.Put uploads outside the lock while backup uploads concurrently — which is where `check`'s 70% spread on identical code comes from. Age once, attach each build in turn, and the layout is held fixed instead of resampled. find gets a pattern nothing matches: its cost is the walk, and matches would add output formatting without adding traversal. diff gets oldest against latest, the whole-tree case; adjacent snapshots differ by one churn step and would measure the churn.
…(RFC 0025) Re-runs "The estimator is load-bearing" on accounting that distinguishes a declared read from one that merely shares a pack, which is the precondition that section set for deleting anything. The totals barely moved, and the breakdown changed the conclusion. Counted per key prefix at 82 backups, 96-100% of every command's unplanned misses are node/: restore, ls, backup and diff have 472 or 473 node reads and one snapshot left, and nothing else. A HAMT descent cannot declare its next read, because which node comes next is a field of the node currently being decoded. So the estimator's job is permanent and it is tree descent, not traversal at large -- which makes deleting packAdmission one well-posed question rather than six vague ones. This section previously predicted the count would move once the accounting was honest: an unknown share of the unplanned misses were meant to be declared reads whose slot had been taken. Measured against a build differing only in that, it moves by 0-3 out of ~500. The mechanism is real and a unit test pins it; its magnitude on these traversals is nil, because a traversal's undeclared reads are node/ and its declared ones are filemeta/, and the two populations barely overlap. Recorded as a wrong prediction rather than quietly dropped. Asking the plan per key rather than per pack costs check 7% and nothing else, and both instruments agree on why: eight whole-pack transfers stop happening that base made on behalf of content objects check never declared, each of which had been populating the body cache and serving around six later content reads as hits. That was a subsidy, not a decision. Whether to keep it is left open, with a third option recorded -- ride a whole fetch the plan will make anyway, while still reporting unplanned and spending nothing. Also corrects §6's variance note. check's 70% spread is a property of the protocol, not of check: each run aged its own repository, and pack composition is not deterministic. Aging once and attaching each build to the same store collapses it -- 712, 712 / 712, 712 / 761, 761 across three builds and two passes, with ls, find and diff identical to the digit in all six.
📝 WalkthroughWalkthroughPack read tracking now uses per-key grouped-read plans for cache hits, misses, and whole-pack admission. Tests cover undeclared reads. RFC 0025 updates measurements. The aging benchmark supports configurable operations and retained repositories. ChangesKey-aware pack read planning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReadPath
participant PackGroupPlan
participant Cache
participant PackStore
ReadPath->>PackGroupPlan: Declare packed object keys
PackStore->>PackGroupPlan: Check whole-pack admission by key
PackStore->>Cache: Read cached pack or object
PackStore->>PackGroupPlan: Consume matching declared key
PackStore->>ReadPath: Return object data
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rfcs/0025-traversal-order-and-pack-contiguous-reads.md (1)
1182-1185: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the K=1 rule as a future policy.
Lines 299-304 state that current ungrouped reads use the estimator. Lines 995-1068 state that the heuristics remain required for unplanned
node/reads. This text instead says ungrouped reads “need no heuristic at all.”State that K=1 ranged reads are a proposed replacement policy. Do not describe it as current behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rfcs/0025-traversal-order-and-pack-contiguous-reads.md` around lines 1182 - 1185, Update the “Grouping covers only the keys a caller hands over” discussion to describe the K=1 arithmetic ranged-read rule as a proposed future policy, not current behavior. Preserve the examples of ungrouped reads and acknowledge that existing estimator heuristics remain required for unplanned node/ reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storelayer/pack.go`:
- Around line 396-409: Preserve one plan instance throughout each Get flow by
capturing the current plan before the cache-miss path calls resolveFromPack.
Update resolveFromPack and the subsequent consume call to use that captured plan
rather than rereading s.plan, ensuring admission and consumption stay tied to
the same declaration; add a regression test that replaces the plan while a
ranged read is blocked.
In `@scripts/benchmark/aging.sh`:
- Around line 313-315: Update the prune handling in the benchmark script so it
runs a full check successfully immediately before invoking prune, regardless of
whether OPS includes check. Apply this consistently to both attach-mode and
normal-mode execution paths, and abort without pruning when validation fails.
---
Outside diff comments:
In `@rfcs/0025-traversal-order-and-pack-contiguous-reads.md`:
- Around line 1182-1185: Update the “Grouping covers only the keys a caller
hands over” discussion to describe the K=1 arithmetic ranged-read rule as a
proposed future policy, not current behavior. Preserve the examples of ungrouped
reads and acknowledge that existing estimator heuristics remain required for
unplanned node/ reads.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3170adb6-ff5c-42d7-b561-6bfbb144e0bb
📒 Files selected for processing (5)
internal/storelayer/pack.gointernal/storelayer/packgroup.gointernal/storelayer/packgroup_test.gorfcs/0025-traversal-order-and-pack-contiguous-reads.mdscripts/benchmark/aging.sh
Comment on lines
+396
to
+409
| s.groupPlan().consume(key, entry.PackRef) | ||
| return data, nil | ||
| } | ||
|
|
||
| return s.resolveFromPack(ctx, key, entry) | ||
| // A miss is a read too. Counting only the cache hits left the plan claiming | ||
| // one object still outstanding on every pack it named — the one whose miss | ||
| // brought the body in — so `count` never reached the zero that retires it, | ||
| // and a pack read entirely by ranged reads kept stating the demand it began | ||
| // with rather than what was left of it. | ||
| data, err := s.resolveFromPack(ctx, key, entry) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| s.groupPlan().consume(key, entry.PackRef) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep one plan instance for each Get call.
Line 405 reads admission state from the current plan. Line 409 consumes the current plan again after external I/O. If PlanReads replaces s.plan while the read is blocked, an old read can consume the declaration from the new plan for the same key. The next declared read then falls back to admission probing.
Capture the plan before resolving the pack. Pass that same plan to resolveFromPack and consume. Add a regression test that replaces the plan while a ranged read is blocked.
Proposed fix
+ plan := s.groupPlan()
if packData, ok := s.packCache.Get(entry.PackRef); ok {
// ...
- s.groupPlan().consume(key, entry.PackRef)
+ plan.consume(key, entry.PackRef)
return data, nil
}
- data, err := s.resolveFromPack(ctx, key, entry)
+ data, err := s.resolveFromPack(ctx, key, entry, plan)
if err != nil {
return nil, err
}
- s.groupPlan().consume(key, entry.PackRef)
+ plan.consume(key, entry.PackRef)
return data, nil
}
-func (s *PackStore) resolveFromPack(ctx context.Context, key string, entry PackEntry) ([]byte, error) {
+func (s *PackStore) resolveFromPack(ctx context.Context, key string, entry PackEntry, plan *packGroupPlan) ([]byte, error) {
// ...
- fetchWhole, planned := s.groupPlan().fetchWhole(key, entry.PackRef)
+ fetchWhole, planned := plan.fetchWhole(key, entry.PackRef)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| s.groupPlan().consume(key, entry.PackRef) | |
| return data, nil | |
| } | |
| return s.resolveFromPack(ctx, key, entry) | |
| // A miss is a read too. Counting only the cache hits left the plan claiming | |
| // one object still outstanding on every pack it named — the one whose miss | |
| // brought the body in — so `count` never reached the zero that retires it, | |
| // and a pack read entirely by ranged reads kept stating the demand it began | |
| // with rather than what was left of it. | |
| data, err := s.resolveFromPack(ctx, key, entry) | |
| if err != nil { | |
| return nil, err | |
| } | |
| s.groupPlan().consume(key, entry.PackRef) | |
| plan := s.groupPlan() | |
| if packData, ok := s.packCache.Get(entry.PackRef); ok { | |
| // ... | |
| plan.consume(key, entry.PackRef) | |
| return data, nil | |
| } | |
| // A miss is a read too. Counting only the cache hits left the plan claiming | |
| // one object still outstanding on every pack it named — the one whose miss | |
| // brought the body in — so `count` never reached the zero that retires it, | |
| // and a pack read entirely by ranged reads kept stating the demand it began | |
| // with rather than what was left of it. | |
| data, err := s.resolveFromPack(ctx, key, entry, plan) | |
| if err != nil { | |
| return nil, err | |
| } | |
| plan.consume(key, entry.PackRef) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/storelayer/pack.go` around lines 396 - 409, Preserve one plan
instance throughout each Get flow by capturing the current plan before the
cache-miss path calls resolveFromPack. Update resolveFromPack and the subsequent
consume call to use that captured plan rather than rereading s.plan, ensuring
admission and consumption stay tied to the same declaration; add a regression
test that replaces the plan while a ranged read is blocked.
Comment on lines
+313
to
+315
| prune) | ||
| measure_op prune "$backups" "$CLOUDSTIC_BIN" prune $flags -quiet | ||
| ;; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the repository before prune.
OPS="" is supported at Line 71. With FINAL_OPS=prune, this path can repack and delete repository data without a preceding full read validation. This affects both attach mode at Line 355 and normal mode at Line 409.
Run a successful full check immediately before prune, even when OPS omits check. Stop when validation fails. As per coding guidelines, “Never allow prune, forget, or repacking to proceed when repository data could not be fully read.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmark/aging.sh` around lines 313 - 315, Update the prune handling
in the benchmark script so it runs a full check successfully immediately before
invoking prune, regardless of whether OPS includes check. Apply this
consistently to both attach-mode and normal-mode execution paths, and abort
without pruning when validation fails.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packGroupPlanheld a count per pack, andconsumeran on every serve from a packed location — so a read nobody declared decremented a declared pack's budget. Packs mix five namespaces, so this is the ordinary case: a traversal declares a batch offilemeta/refs and then reads, undeclared, thecontent/objects those refs name, thenode/objects it discovers mid-walk, and whatever path resolution needs — a good share of them out of the packs it just declared against.PlanReadsalready materialises every key to sort it, so the set costs a map slot apiece over the caller's own strings — bounded by the batch (8,192 refs for every streaming traversal), never by the catalog.consumespends only a declared key, andfetchWholeis asked per key rather than per pack.consumenow also runs on the miss path. Counting only cache hits left every named pack claiming one object outstanding — the one whose miss brought the body in — so a plan never reached the zero that retires it.aging.shgainsls,findanddiff;backupandprunemove to a separateFINAL_OPSbecause they change the repository under the curve; andATTACH=1/KEEP_STORE=1let two builds be measured against one repository.What the re-run found
This is the precondition RFC 0025 set on re-running the planned-versus-unplanned count in "The estimator is load-bearing". Counted at 82 backups against MinIO, per key prefix:
restorenode/472,snapshot/1checknode/577,content/118lsnode/472findnode/632,filemeta/24backupnode/473prunenode/4,52396–100% of every command's unplanned misses are
node/.restore,ls,backupanddiffhave 472 or 473 node reads and one snapshot left, and nothing else. A HAMT descent cannot declare its next read — which node comes next is a field of the node currently being decoded. SopackAdmission's remaining job is tree descent, not traversal at large, which turns "can we delete it" into one well-posed question instead of six.The RFC's stated reason for expecting this to matter was wrong, and is recorded as such. It predicted the count would move once the accounting was honest. Measured against a build differing only in that, it moves by 0–3 out of ~500 — a traversal's undeclared reads are
node/and its declared ones arefilemeta/, so the two populations barely overlap. The defect is real and a unit test pins it; its effect on the number was nil. The breakdown, not the fix, is what changed the conclusion.One open decision for the reviewer
Making the plan name its keys is two changes, and only one is free. Three builds, one repository, two passes each —
base,mid(key-awareconsumeonly),fix(alsofetchWholeper key):restorechecklsfinddiffThe accounting fix costs nothing. Asking per key costs
check7% and nothing else. A synchronous counter and a-debugtrace of the same process agree prefix by prefix on why: eight whole-pack transfers stop happening thatbasemade on behalf ofcontent/objectschecknever declared, each of which had been populating the body cache and serving ~6 later content reads as hits. That was a subsidy, not a decision — the plan answering for reads it was never told about, the same conflation that let an undeclared read spend a declared budget.Keeping it also closes the one-plan-per-
PackStorehazard: acatof one file inheriting another operation's eighty-object whole-fetch decision transfers 8 MB to return a few hundred bytes. If 49 requests oncheckis the wrong trade, say so and I will shipmidinstead. A third option is recorded in the RFC and not built here: let an undeclared read ride a whole fetch the plan is going to make anyway, while still reporting unplanned and spending nothing.Related issues
None — RFC 0025 has no tracking issue. Part of RFC 0025.
Repository compatibility
No repository format change. This changes only how
PackStoredecides between a ranged read and a whole-pack transfer; the objects read and returned are identical either way, and nothing here is written to a store.Verification
env GOCACHE=/tmp/cloudstic-gocache go test -race -count=1 ./internal/storelayer ./internal/engine ./pkg/store env GOCACHE=/tmp/cloudstic-gocache GOLANGCI_LINT_CACHE=/tmp/cloudstic-golangci-lint golangci-lint run ./internal/... ./pkg/...Both pass;
golangci-lintreports 0 issues. Fullgo test -race -count=1 ./...also passes.Two regression tests, both confirmed failing on the pre-change code:
TestPackGroupPlan_UndeclaredReadKeepsDeclaredBudget— before:filemeta/1 is no longer planned after 4 undeclared reads of packs/a.TestPackStore_UndeclaredReadDoesNotSpendAPlannedPacksBudget— before:7 ranged reads serving 40 declared keys, want 0.Request counts are from
scripts/benchmark/aging.shagainst MinIO at 80 backups / 5,000 files / 82 packs. Not frombench.sh, which builds a fresh repository per cell and structurally cannot see aged-repository behaviour, and whosecheckcell has a documented 70% run-to-run spread. Holding one repository fixed and attaching each build collapses that spread to 0% — the six read rows above reproduce to the digit — which is what theATTACHchange is for.pruneis reported from a single sample and its 65% spread stands; it is not treated as an A/B.Documentation
No documentation change required. No exported API changes:
packGroupPlanis unexported andPackStore's public surface is untouched, so the separate docs repository is unaffected. RFC 0025 is updated in this PR.Summary by CodeRabbit
Bug Fixes
Benchmarking
Documentation