docs(rfc-0011): A1 re-scope — template-mining compression is logical, not byte-level - #193
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR reclassifies A1 from a gating compression thesis to a diagnostic measurement via RFC 0011, updates RFC 0001 to reflect that ChangesA1 Reclassification to Diagnostic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Docs-only tuning RFC update that re-scopes thesis gate A1 (compression vs zstd) from a blocking validated gate to a recorded diagnostic, based on new HDFS_v1 measurements, and updates the benchmark/RFC documentation accordingly.
Changes:
- Add RFC 0011 documenting the A1 re-scope decision and supporting measurements.
- Update
docs/benchmarks.mdto mark A1 as diagnostic (not gating) and record the HDFS_v1 A1/C1/C2 run in a new results section. - Update RFC 0001’s status note to reflect the new
validatedpath (C1 + C2; A1 diagnostic), and add the RFC to the mdBook SUMMARY.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/SUMMARY.md | Adds RFC 0011 to the mdBook navigation. |
| docs/rfcs/0011-a1-rescope.md | New tuning RFC defining A1 as diagnostic and documenting the HDFS_v1 evidence. |
| docs/rfcs/0001-template-miner.md | Updates the validated path note to reflect the re-scoped gates (C1/C2) and A1’s diagnostic status. |
| docs/benchmarks.md | Marks A1 diagnostic in the thesis-gate table and adds a new §9.5 HDFS_v1 results entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/benchmarks.md (1)
16-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake A1 consistently diagnostic everywhere in this document.
The opening status blurb still calls A1 the open gate, and the A1 summary section / thesis-gate table still frame it as a
[THESIS]goal. That conflicts with §9.5 and RFC 0011, which demote A1 to diagnostic-only. Please update those sections together so the benchmark doc stays internally consistent.Also applies to: 94-109, 337-351
🤖 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 `@docs/benchmarks.md` around lines 16 - 18, Update the document so A1 is consistently marked diagnostic-only: in the opening status blurb remove the phrase "open gate" for A1 and instead state it is diagnostic-only, change the A1 summary section and the thesis-gate table to remove the [THESIS] designation and mark A1 as diagnostic/diagnostic-only, and ensure any other references to A1 (e.g., in the A1 summary paragraph and any gate-status listings) match §9.5 and RFC 0011's demotion of A1 to diagnostic-only; search for all occurrences of "A1", "open gate", "[THESIS]" and the thesis-gate table and update them to the diagnostic-only wording so the document is internally consistent.
🤖 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.
Outside diff comments:
In `@docs/benchmarks.md`:
- Around line 16-18: Update the document so A1 is consistently marked
diagnostic-only: in the opening status blurb remove the phrase "open gate" for
A1 and instead state it is diagnostic-only, change the A1 summary section and
the thesis-gate table to remove the [THESIS] designation and mark A1 as
diagnostic/diagnostic-only, and ensure any other references to A1 (e.g., in the
A1 summary paragraph and any gate-status listings) match §9.5 and RFC 0011's
demotion of A1 to diagnostic-only; search for all occurrences of "A1", "open
gate", "[THESIS]" and the thesis-gate table and update them to the
diagnostic-only wording so the document is internally consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d06b98e-4b12-4a80-ae4c-878a07d255b0
📒 Files selected for processing (4)
docs/SUMMARY.mddocs/benchmarks.mddocs/rfcs/0001-template-miner.mddocs/rfcs/0011-a1-rescope.md
… not byte-level A1 (compression >= 3x over zstd-alone) was only ever measured on OTel-Demo (best 0.829x). Running it on LogHub HDFS_v1 — 11.2M lines, the maximally-templated corpus, the single best case for the premise — fails harder (0.516x, section 9.5): the more templated the corpus, the more completely monolithic zstd-19 captures its redundancy (16x), while template mining's extracted params are high-cardinality columns that don't compress as well and columnar Parquet adds framing. The best case for template mining is the best case for the byte codec. RFC 0011 (tuning RFC, benchmarks.md section 7 escalation path) demotes A1 from a gating thesis criterion to a recorded diagnostic, and recognises template mining's compression value as logical/query-pruning — captured by B1/B2 (RFC 0007, validated). The template-miner pillar's gates become C1 + C2, both of which PASS on representative HDFS_v1 (C1 1.000000; C2 a 40-template plateau at 11.2M lines, formal gate applying). RFC 0001's validated path is re-scoped accordingly (status stays green pending RFC 0011 acceptance + an authoritative baseline rerun). Also: benchmarks.md section 7 gate table marks A1 diagnostic; 9.5 records the HDFS A1/C1/C2 diagnostic run; SUMMARY nav; RFC 0001 status note. A CLAUDE.md section 2 pillar-2 wording clarification is flagged as a meta-RFC follow-up, not enacted here. The maturity-ladder flips are the maintainer's. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jensholdgaard
force-pushed
the
docs/rfc0011-a1-rescope
branch
from
June 13, 2026 21:55
25c9430 to
4e84615
Compare
| > computed and written to the §9 results, flagged diagnostic — so the | ||
| > queryability premium stays visible and codec regressions surface | ||
|
|
||
| ## 6. Measurements (2026-06-13, local — `hardware_kind = "unknown"`) |
Comment on lines
+731
to
+734
| **Local hardware → diagnostic, not | ||
| authoritative**; A1's verdict is corpus-structural and | ||
| hardware-independent (compressed bytes are deterministic), C1/C2 are | ||
| ratios, so the findings hold regardless of the runner. |
Comment on lines
+169
to
+172
| these are **diagnostic**, not the authoritative `baseline-8vcpu-32gib` | ||
| numbers; A1's verdict is corpus-structural and hardware-independent | ||
| (compressed bytes are deterministic), and C1/C2 are ratios, so the | ||
| finding stands regardless of the runner. The authoritative |
jensholdgaard
added a commit
that referenced
this pull request
Jun 14, 2026
… clock (#197) * test(ingester): make RFC0008.8 latency test deterministic via virtual clock `rfc0008_8_p99_latency_tracks_batch_window` asserted a strict 3-way monotonic ordering of three *wall-clock* P99 measurements (`p99(10) < p99(50) < p99(150)`). That ordering is non-deterministic: on a loaded or instrumented runner the per-flush fixed overhead dominates the smallest window and any sample can spike, flipping the order. It flaked the required `cargo test` job on #193 (`[164, 491, 311]`) and the `continue-on-error` coverage job on #196 (`[482, 53, 154]`, llvm-cov instrumentation inflating the 10 ms window to 482 ms). Rewrite it to run under a paused virtual clock (`#[tokio::test(start_paused = true)]`, enabled by tokio's `test-util` dev feature). tokio auto-advances to the next pending timer when the runtime is idle, so the only time that elapses is the coordinator's own `tokio::time::sleep(window)`; the real fsync (offloaded to `spawn_blocking`) runs in wall-clock time but does not advance the virtual clock. Measuring with `tokio::time::Instant` therefore yields the commit's batch wait exactly, with zero jitter. This strengthens the contract rather than weakening it (§6.2): a batch of commits fired together all ride one window, so ack latency *equals* the configured window and scales 1:1 across the spec's real `{10, 100, 1000}` ms values (now free of wall-clock cost — the prior `{10, 50, 150}` deviation is gone). A per-record-fsync impl would ack at ≈ 0 regardless of the window. Batching itself (`appends_per_sync ≫ 1`) stays pinned counter-exactly by the sibling test. Approach chosen by the maintainer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(rfc-0008): align §5 RFC0008.8 with the virtual-clock test Copilot flagged a spec/test mismatch on #197: the RFC0008.8 §5 scenario still described measuring wall-clock P99 ack latency over a 10 s sample within a ±30 % tolerance, but the test now asserts exact ack-latency equality under a paused virtual clock. Update the scenario (and the §5 summary) to the virtual-clock formulation: a batch of commits fired together under a paused clock each ack at exactly the configured window (deterministic, no jitter) — the window dominates, not per-record fsync. The contract is unchanged (window dominates, appends_per_sync ≫ 1, §3.4 holds); only the measurement method is restated, superseding the non-deterministic wall-clock-P99 formulation. Approach approved by the maintainer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 14, 2026
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
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.
What
A tuning RFC (
benchmarks.md§7 escalation path) that reconciles the A1 thesis gate with the evidence from the bigger-corpus investigation, and re-scopes RFC 0001's path tovalidated. Docs-only.The finding
A1 ("ourios on-disk bytes ≥ 3× smaller than zstd-19 over raw") had only ever been measured on the OTel-Demo corpus class (best 0.829×). I ran it on LogHub HDFS_v1 — 11.2M lines, the maximally-templated corpus, the single best case for the "template mining → 50–200× compression" premise:
It fails harder on the best case. Mechanism: the more templated the corpus, the more completely monolithic zstd-19 captures its redundancy in one window (16×); template mining's extracted params (block IDs, timestamps, IPs) are high-cardinality columns that don't compress as well, and columnar Parquet adds framing that's the price of queryability. The best case for template mining is the best case for the byte codec it's measured against — so
≥ 3× over zstdcannot hold on any realistic log corpus.The decision (maintainer-chosen: tuning RFC)
delta < 3.0×no longer blocks any RFC'svalidated.validated) — not on-disk bytes.validatedthesis gates become C1 + C2, both of which PASS on representative HDFS_v1 (same run): C1 =1.000000reconstruction (11,175,578 rows, 51 lossy); C2 = 40-template plateau at 11.2M lines with the formal gate applying (≥ 1 M, not abstaining as on the sub-1M OTel runs).What this PR does vs. defers
drafted); marks A1 diagnostic in the §7 gate table; records the HDFS A1/C1/C2 run in a new §9.5 (local/unknownhardware — clearly diagnostic, not the authoritative baseline); re-scopes RFC 0001's status-notevalidatedpath; SUMMARY nav.validated(gated on RFC 0011 acceptance + an authoritativebaseline-8vcpu-32gibC1/C2 rerun — verdicts are deterministic, not expected to change); aCLAUDE.md§2 pillar-docs: apply RFC maturity-model amendments #2 wording clarification (flagged as ameta:RFC per CLAUDE.md's own rule, not touched here).Invariants / honesty
This is exactly the "honest handling of where template mining can go wrong" the project calls for: the thesis (
CLAUDE.md§2 pillar #2) is sound, but A1 measured the wrong quantity. The HDFS corpus is fetched at bench time and never redistributed (it stays in gitignoredscratch/).Checks
mdbook buildclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation