Mark KeyViz hot-key Top-K implemented#1030
Conversation
|
@codex review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates references to the KeyViz hot-key Top-K design document across various design documents, source code comments, and configuration files, changing them from "proposed" to "implemented". The reviewer suggested also updating the reference to the sub-range sampling design document in docs/design/2026_06_11_proposed_hotspot_split_milestone3_automation.md to its "implemented" version for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - The `splitRange` it would call is midpoint-only (`midpoint(r.Start, r.End)` appends a `0x00` byte, `distribution/engine.go:541-547`) and mutates the **in-memory** engine routes directly, bypassing the catalog. That violates the repo convention that all catalog mutations go through `SplitRange` so the version bumps and watchers fan out (CLAUDE.md "Route catalog mutations must go through `SplitRange`"). It also contradicts the parent doc §3.1.3 requirement "Choose split keys from observed key distribution (not midpoint-only)." | ||
|
|
||
| Meanwhile, a **second**, fully-wired load-observation pipeline already exists: the keyviz sampler (`keyviz/sampler.go`). The coordinator calls `Sampler.Observe` per resolved `(RouteID, key)` on both the write path (`groupMutations → observeMutation`, `kv/sharded_coordinator.go:1844` and `:1795-1800`) and the linearizable/lease read path (`observeRead`, `kv/sharded_coordinator.go:1819-1824`). The sampler keeps per-route read/write op counts and byte counts, windowed at a flush interval (`keyviz/flusher.go:19` `RunFlusher`), with an optional per-route Top-K hot-key sketch (`docs/design/2026_05_28_proposed_keyviz_hot_key_topk.md`) and order-preserving sub-range buckets (`docs/design/2026_05_25_proposed_keyviz_subrange_sampling.md`). It is allocation-free on the hot path and already behind `--keyvizEnabled`. | ||
| Meanwhile, a **second**, fully-wired load-observation pipeline already exists: the keyviz sampler (`keyviz/sampler.go`). The coordinator calls `Sampler.Observe` per resolved `(RouteID, key)` on both the write path (`groupMutations → observeMutation`, `kv/sharded_coordinator.go:1844` and `:1795-1800`) and the linearizable/lease read path (`observeRead`, `kv/sharded_coordinator.go:1819-1824`). The sampler keeps per-route read/write op counts and byte counts, windowed at a flush interval (`keyviz/flusher.go:19` `RunFlusher`), with an optional per-route Top-K hot-key sketch (`docs/design/2026_05_28_implemented_keyviz_hot_key_topk.md`) and order-preserving sub-range buckets (`docs/design/2026_05_25_proposed_keyviz_subrange_sampling.md`). It is allocation-free on the hot path and already behind `--keyvizEnabled`. |
There was a problem hiding this comment.
The reference to the sub-range sampling design should be updated to its implemented filename (2026_05_25_implemented_keyviz_subrange_sampling.md) since that design has also been implemented (as noted in the PR description and other updated files).
| Meanwhile, a **second**, fully-wired load-observation pipeline already exists: the keyviz sampler (`keyviz/sampler.go`). The coordinator calls `Sampler.Observe` per resolved `(RouteID, key)` on both the write path (`groupMutations → observeMutation`, `kv/sharded_coordinator.go:1844` and `:1795-1800`) and the linearizable/lease read path (`observeRead`, `kv/sharded_coordinator.go:1819-1824`). The sampler keeps per-route read/write op counts and byte counts, windowed at a flush interval (`keyviz/flusher.go:19` `RunFlusher`), with an optional per-route Top-K hot-key sketch (`docs/design/2026_05_28_implemented_keyviz_hot_key_topk.md`) and order-preserving sub-range buckets (`docs/design/2026_05_25_proposed_keyviz_subrange_sampling.md`). It is allocation-free on the hot path and already behind `--keyvizEnabled`. | |
| Meanwhile, a second, fully-wired load-observation pipeline already exists: the keyviz sampler (keyviz/sampler.go). The coordinator calls Sampler.Observe per resolved (RouteID, key) on both the write path (groupMutations -> observeMutation, kv/sharded_coordinator.go:1844 and :1795-1800) and the linearizable/lease read path (observeRead, kv/sharded_coordinator.go:1819-1824). The sampler keeps per-route read/write op counts and byte counts, windowed at a flush interval (keyviz/flusher.go:19 RunFlusher), with an optional per-route Top-K hot-key sketch (docs/design/2026_05_28_implemented_keyviz_hot_key_topk.md) and order-preserving sub-range buckets (docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md). It is allocation-free on the hot path and already behind --keyvizEnabled. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10634665aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| phase: 2-A++ | ||
| parent_design: docs/admin_ui_key_visualizer_design.md | ||
| related_design: docs/design/2026_05_25_proposed_keyviz_subrange_sampling.md | ||
| related_design: docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md |
There was a problem hiding this comment.
In this commit, the tree still contains only docs/design/2026_05_25_proposed_keyviz_subrange_sampling.md for the subrange design, so this new ...implemented... reference (also repeated in the body link) points at a non-existent file unless the separate dependency is always applied first. That breaks design metadata/navigation and any link checking for this commit as merged on its current parent.
Useful? React with 👍 / 👎.
|
@codex review |
401fc4a to
a5c74c9
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
f4e4052
into
design/promote-keyviz-subrange-sampling-doc
1 participant
Author: bootjp
Summary
Dependency
Verification