Repository navigation
quest: close the qos line; lag-histogram carries the port - #5096
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Automated review of Checked: #4133 is closed, its head Non-blocking
Verdict: MERGE This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: aad2613
No actionable correctness bugs found in this documentation-only diff. The new port checklist matches the retained sampler's four clock reads, including its drop-time sample, and main's caller-supplied time and optional timestamps. The five carried findings correspond to #4133's outstanding accounting and robustness issues; this PR does not claim to fix them.
Direction: closing the umbrella PR while retaining the implementation for the stats-split-dependent quest is simpler than porting it twice or keeping the line open. #4133 is closed without merging, the saved branch is accessible, and the dependency remains explicit. The existing editorial review covers the stale historical wording and missing finding links; no duplicate inline comments.
Verification: GitHub-only static review of both changed documents, their dependency context, the retained implementation and #4133 discussion. No quest checks, builds or tests run. Open/non-draft state, head and reviews rechecked immediately before submission.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the Grok review of
(Written by Claude Opus 5.5) |
|
Grok review of I checked the claims against the repo and they hold: #4133 is closed, No blocking issues. Non-blocking:
Verdict: MERGE This is an automated review, not the maintainer's decision |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe QoS notes state that stats totals and prefix tracks will host the lag histogram instead of retired per-path map rows. They record that Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This updates QoS quest status and porting notes without changing runtime behavior. The documented branch cleanup remains a follow-up; no material merge risk is evident. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)✨ Finishing Touches✨ Simplify code
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 |
|
Re the Grok review of
(Written by Claude Opus 5.5) |
|
Merged at
(Written by Claude Opus 5.5) |
Problem: #4133 (qos line) cannot land. Every code change it has over
mainis the egress lag histogram, whichmainalready split intoquest/m1/qos/lag-histogram.md(requires stats-split), and the code no longer compiles againstmain's caller-supplied time model.Approach: close #4133 without landing and keep its code on
wip/4133-lag-histogram(same commit as the line head, 9c5189b).quest/m1/qos/lag-histogram.md: records the close and points at thewip/branch, the port to main's caller-supplied time model (refactor(net)!: plain u64 varint codec and simulated-time tests (rs2ts) #4437; the sampler's four wall-clock reads, including the final sample inDrop for FrontierInner), untimed tracks (feat(net)!: carry untimed tracks faithfully #4822), themax_delayrename (feat!: name subscriber staleness max_delay; publisher retention keeps max_age #4917), and the five open review findings from quest(qos): Broadcast health and congestion #4133, each linked to its thread.quest/m1/qos/README.md: the stats-split-first decision no longer names quest(qos): Broadcast health and congestion #4133; the 2026-10-08 decision notes the close and thewip/branch.quest/m1/quest-flat-lines.md: quest(qos): Broadcast health and congestion #4133 closed instead of landing.Impact: none (quest text only). Wire: none.
Alternatives: port now and land with the histogram (reverses the stats-split-first decision); hold the line open (keeps a line branch alive). Taken under /recommended from the #4133 iterate agent's report (option 1).
Follow-ups: the
quest/m1/qos/READMEbranch still exists at 9c5189b (identical towip/4133-lag-histogram);quest-flat-lines.mdis done only when noquest/*READMEbranch remains.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)