Mark KeyViz cluster fanout implemented#1025
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughKeyVizクラスターファンアウトの実装版設計を追加し、HTTPピア照会・サーバー側マージ・劣化応答・競合表示を定義した。管理UI設計、関連ドキュメント、コードコメントの参照先を実装版へ更新し、提案版設計を削除した。 ChangesKeyVizクラスターファンアウト
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
@codex review |
![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 the status of the keyviz cluster fan-out design document from "proposed" to "implemented," renaming the file and updating all references to it across other design documents, source code comments, and configuration files. It also refines some of the language in the design document to generalize references to previous review findings. There are no review comments, so I have no feedback to provide.
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.
![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: ebde63c255
ℹ️ 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".
|
@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". |
|
@codex review |
2270fa6 to
dfc61d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfc61d341f
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
対応しました。
検証:
|
There was a problem hiding this comment.
💡 Codex Review
Use the real admin listener flag
This implemented rollout example still uses --adminAddress, but the binary defines the admin HTTP listener flag as --adminListen (main.go) and the fan-out self filtering is built from that listener value. An operator copying this implemented config will start the process with an unknown flag, and the surrounding self-match bullets repeat the wrong flag name, so the documented Phase 2-C rollout cannot be followed as written.
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 250 to 251 in 32a6e4f
Don't claim timestamp bucketing is implemented
This implemented spec promises peers round column_unix_ms to the keyvizStep boundary before merging, but the current fan-out merge only unions the raw ColumnUnixMs values via unionColumns and no step duration is available in mergeKeyVizMatrices. In clusters whose flushes land at slightly different millisecond offsets, one logical window will still appear as multiple columns despite the doc saying routine jitter is coalesced; either document exact-timestamp union or add the rounding before presenting this as shipped behavior.
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 223 to 224 in 32a6e4f
Don't promise config-order metadata selection
The implemented doc says row metadata comes from the lowest-indexed node in --keyvizFanoutNodes, but KeyVizFanout.Run always merges []KeyVizMatrix{local} before appending peer matrices, so the serving node wins metadata even when it appears later in the shared flag list. During catalog divergence this makes Start/End and route metadata depend on which node served the browser request, contrary to the deterministic rule documented here.
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 35 to 36 in 32a6e4f
Don't say old SPAs keep rendering the local view
When --keyvizFanoutNodes is enabled, the server rewrites the existing rows/values payload to the merged cluster matrix; an old SPA may ignore the new fanout, conflict, and identity fields, but it will still render the aggregated values rather than the local view. This rollout note can mislead mixed-version validation because the real compatibility risk is that old clients show cluster-wide data without the new degraded/conflict affordances.
ℹ️ 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".
|
対応内容:
検証:
|
There was a problem hiding this comment.
💡 Codex Review
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 230 to 231 in b1a34d5
Defer catalog-divergence warnings consistently
For same-BucketID rows with different Start/End, this section still says the divergence is surfaced as a warning in the per-node status payload, but the implemented FanoutNodeStatus only serializes node, ok, and error, and §5 now explicitly says catalog-divergence warnings are not shipped in Phase 2-C. This leaves the implemented design promising an operator-visible signal that the wire format cannot carry, so this should be reworded as a future extension or backed by an actual warning field.
Use the implemented admin listener flag
This implemented configuration example uses --adminAddress, but the server flag is --adminListen (main.go defines flag.String("adminListen", ...)), so operators copying the shipped fan-out doc get an unknown flag before the server starts. The self-matching prose below repeats the same name, so update the example and references to the actual listener flag.
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 236 to 238 in b1a34d5
Stop claiming column timestamps are aligned
When peer flush times differ by sub-step jitter, this implemented doc promises the aggregator bucket-aligns timestamps to keyvizStep, but the current merge path builds unionColumns from the exact ColumnUnixMs values and KeyVizFanout is not given keyvizStep to round with. Operators will expect skew tolerance that the shipped code does not provide, so either implement the rounding before merging or document the exact-timestamp union behavior.
ℹ️ 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".
|
対応内容:
検証:
|
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
…-cluster-fanout-doc # Conflicts: # docs/design/2026_06_23_proposed_scaling_roadmap.md
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 104eb4f47b
ℹ️ 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".
| reserved 3, 4, 5; | ||
| reserved "gated", "max_lag", "target_candidates"; |
There was a problem hiding this comment.
The active leader-balance design still requires RaftAdminTransferLeadershipRequest to add bool gated = 3, uint64 max_lag = 4, and repeated TransferTarget target_candidates = 5 for forwarded, catch-up-gated leadership transfers (docs/design/2026_06_11_proposed_leader_balance_scheduler.md:111 and :242). Reserving exactly those numbers/names here makes protoc reject that documented wire-compatible extension, forcing a renumbering that loses the mixed-version behavior the design depends on, so these reservations should be removed or the design updated before landing.
Useful? React with 👍 / 👎.
| - Fan-out aggregator with the §4 merge rules, degraded peer status, | ||
| peer timeouts, bounded peer response bodies, cookie forwarding, and |
There was a problem hiding this comment.
This promotes the fan-out aggregator as implementing all §4 merge rules, but §4.5's timestamp bucketing is still not in the server: pivotKeyVizColumns emits raw col.At.UnixMilli() and unionColumns unions exact timestamps rather than rounding to keyvizStep. In clusters whose flush ticks differ by even small sub-step offsets, peer samples still split into adjacent columns, so the documented cluster heatmap guarantee is false unless the rounding is implemented or this part remains deferred.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dbadac3e7
ℹ️ 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".
| --adminListen 127.0.0.1:8080 \ | ||
| --keyvizEnabled \ | ||
| --keyvizFanoutNodes=10.0.0.1:8080,10.0.0.2:8080,10.0.0.3:8080 |
There was a problem hiding this comment.
When this example is copied for a multi-node rollout, it does not actually expose a peer admin endpoint: the HTTP admin server is disabled by default unless --adminEnabled is set, and binding --adminListen to 127.0.0.1 makes the documented 10.0.0.x:8080 fan-out endpoints unreachable from other hosts and not self-filtered by buildKeyVizFanout's exact/wildcard matching. The rollout doc should show the required admin enablement plus a reachable per-node listen address (with the necessary TLS or plaintext opt-in flags) so operators do not get only connection-refused/degraded fan-out responses.
Useful? React with 👍 / 👎.
| - sets `conflicts[j]=true` only when two sources report different | ||
| non-zero values for the same `(group, term, column)` identity. |
There was a problem hiding this comment.
For mixed-version or legacy peers this "only" condition is not what the shipped merge reports: cellMergeAcc.hasConflict returns true on the unknown-identity fallback path when distinct non-zero values are seen, and TestMergeKeyVizMatricesGroupTermFallbackOnUnknownTerm asserts Conflicts[j] in that case. As written, operators could treat a rollout-era known+legacy disagreement as an impossible same-term Raft violation, so the docs should include the fallback conflict case alongside the same-identity case.
Useful? React with 👍 / 👎.
| - Each entry is a host:port. TLS is out of scope for Phase 2-C | ||
| (intra-cluster admin traffic is assumed to ride a private |
There was a problem hiding this comment.
For deployments that enable admin TLS (the normal non-loopback configuration unless --adminAllowPlaintextNonLoopback is set), following this guidance makes fan-out call http://host:port against an HTTPS listener and every peer will degrade. The implementation already preserves explicit schemes in buildKeyVizPeerURL, and main.go documents host:port or scheme://host:port, so the implemented rollout doc should tell TLS users to configure https://... peer endpoints rather than saying TLS is out of scope.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c55050113c
ℹ️ 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".
| will not match — the fan-out makes a loopback HTTP call to | ||
| itself. This is harmless (it degrades to one extra round-trip | ||
| per request) but wasteful; operators should prefer the literal | ||
| `--adminAddress` value in the flag. | ||
| `--adminListen` value in the flag. |
There was a problem hiding this comment.
When this branch is hit because --keyvizFanoutNodes names the same process through a DNS name or reverse proxy, the peer request returns the same local matrix with the recursion guard set, and the merge path sums reads/read_bytes across sources. That means these self-loop calls double-count local read traffic rather than just adding one wasted round trip, so operators using aliases need either an explicit self name to filter or instructions to omit that alias from the fan-out list.
Useful? React with 👍 / 👎.
| must use explicit `https://host:port` entries: | ||
| `--keyvizFanoutNodes=https://node1.internal:8443,https://node2.internal:8443`. | ||
| The URL builder preserves explicit schemes, so TLS fan-out does not |
There was a problem hiding this comment.
This says TLS fan-out only needs an https:// URL, but NewKeyVizFanout uses the default http.Client and the fan-out config has no CA bundle or insecure-dev option, so peer certificates must already chain to the process/system trust roots. In common admin-TLS deployments using self-signed or private-CA certs that are not installed in the container trust store, every peer call fails certificate verification and the cluster view stays degraded; either document that trust-store prerequisite here or add an explicit CA/trust configuration before promising no separate flag.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eee0232c6
ℹ️ 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".
| Because writes are recorded by Raft leaders and follower-local reads are recorded by the followers that serve them (§5.1), a single node's local admin view produces a **partial heatmap**. Phase 2-C ships a narrower embedded fan-out path inside each server process: | ||
|
|
||
| - `--nodes` accepts a comma-separated list of seed addresses. The admin binary calls `GetClusterOverview` on any reachable seed to discover the current full membership (node → gRPC endpoint, plus per-group leader identity). Membership is cached for `--nodesRefreshInterval` (**default 15 s**) so a stampede of concurrent browser requests hits at most one `GetClusterOverview` per interval per seed, while scale-out events are still reflected within seconds. The cache is refreshed lazily on the first request after expiry and invalidated immediately on any per-node `Unavailable` error, so removed or replaced nodes are dropped on the next request instead of waiting for the next tick. | ||
| - For each query (`GetKeyVizMatrix`, `GetRouteDetail`, `GetAdapterSummary`), the admin binary issues parallel gRPC calls to every known node and merges results server-side before sending one combined JSON payload to the browser. | ||
| - Merging rule for the heatmap: rows are grouped by `bucketID`/`lineageID` and time step. Read samples from multiple nodes are **summed**, because they represent distinct locally served reads. For write samples the authoritative identity is `(raftGroupID, leaderTerm)` — by Raft invariants at most one leader exists per term per group — so the admin binary collapses write samples to **one value per `(bucketID, raftGroupID, leaderTerm, windowStart)`** key. If the same logical key arrives from more than one node (e.g., an ex-leader that has not yet expired its local cache plus a correctly-responding new leader in the same term), the entries are expected to be identical and the merger keeps one; if they differ, the cell is surfaced with `conflict=true` (not silently dropped). Across distinct `leaderTerm` values for the same group and window, values are summed because each term's leader only observed its own term's writes. The admin binary never uses "later timestamp wins" to overwrite a previous leader's complete window with a new leader's partial window. | ||
| - Degraded mode: if any node is unreachable, the admin binary returns a partial result with a per-node `{node, ok, error}` status array so the UI can surface "3 of 4 nodes responded" instead of silently hiding ranges. The heatmap hatches rows or time windows whose expected source node failed. | ||
| - A single-node mode — pass one address to `--nodes` and the admin binary will fan out to just that node's view. A future `--no-fanout` flag that also suppresses the background membership-discovery RPC is deferred; for now the operator can simulate it by pointing at a single seed and accepting the one-node partial view. | ||
| - `--keyvizFanoutNodes` accepts a comma-separated static list of peer admin HTTP endpoints. Empty disables fan-out. The local node is added in-process, and the configured peers are queried over the existing admin HTTP surface with the `X-Admin-Fanout-Peer` recursion guard. Phase 2-C does not use `cmd/elastickv-admin --nodes`, `GetClusterOverview` membership discovery, or `--nodesRefreshInterval`; dynamic discovery remains a future extension. |
There was a problem hiding this comment.
After this section switches Phase 2-C to the embedded HTTP handler, the deployment bullets just above still tell operators to run elastickv-admin --nodes=... --nodeTokenFile and say browser login/RBAC are deferred. That standalone binary is only a Phase-0 gRPC-token overview surface, so following that workflow will not serve the implemented KeyViz SPA/fan-out path or its cookie-login prerequisites; please update the operator workflow alongside this embedded implementation note.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
@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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/admin_ui_key_visualizer_design.md (1)
342-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPhase 2-C の完了条件を部分結果の契約に合わせてください。
§9.1 および実装版設計では、ノード障害時に
ok=falseの状態を付けた劣化した部分結果を返します。「1 ノード停止でも complete view」は実装契約と矛盾し、ロールアウト時に欠落データを完全ビューとして扱う誤解を招きます。「partial cluster view with per-node failure status」などに修正してください。🤖 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/admin_ui_key_visualizer_design.md` around lines 342 - 343, Phase 2-C の完了条件を、ノード障害時に完全ビューではなく劣化した部分結果を返す実装契約に合わせて修正してください。該当する「1 node down でも complete view」の記述を、「per-node failure status を含む partial cluster view」など、部分結果とノード単位の失敗状態を明示する表現へ変更してください。scripts/rolling-update.env.example (1)
114-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winファンアウトの認証・URL 契約を実装版に一致させてください。
この説明は、
--adminEnabledなしのピアが未認証 KeyViz エンドポイントを公開すると記載していますが、実装版設計では--adminEnabledが必須で、未設定ならハンドラ自体がマウントされません。誤った説明は、管理認証を設定せずに運用できるという危険な誤解を招きます。あわせて、TLS 構成で必要なhttps://host:port形式もこの例に明記してください。🤖 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/rolling-update.env.example` around lines 114 - 126, Update the KEYVIZ_FANOUT_NODES documentation to state that every peer must run with --adminEnabled and that the KeyViz handler is not mounted when it is disabled; remove the claim that peers without it expose an unauthenticated endpoint. Also document that TLS configurations must use https://host:port entries in the comma-separated list.
🤖 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 `@docs/admin_ui_key_visualizer_design.md`:
- Around line 79-82: §3 のアーキテクチャ図と説明を更新し、standalone overview は管理バイナリから各ノードへ gRPC
接続する経路、KeyViz Phase 2-C の fan-out は各 elastickv に埋め込まれた HTTP
ハンドラを利用する経路として明確に分離してください。§9.1 の embedded HTTP handler
への参照を追加し、図中の接続方式・データ取得経路も実装と一致させてください。
---
Outside diff comments:
In `@docs/admin_ui_key_visualizer_design.md`:
- Around line 342-343: Phase 2-C
の完了条件を、ノード障害時に完全ビューではなく劣化した部分結果を返す実装契約に合わせて修正してください。該当する「1 node down でも complete
view」の記述を、「per-node failure status を含む partial cluster
view」など、部分結果とノード単位の失敗状態を明示する表現へ変更してください。
In `@scripts/rolling-update.env.example`:
- Around line 114-126: Update the KEYVIZ_FANOUT_NODES documentation to state
that every peer must run with --adminEnabled and that the KeyViz handler is not
mounted when it is disabled; remove the claim that peers without it expose an
unauthenticated endpoint. Also document that TLS configurations must use
https://host:port entries in the comma-separated list.
🪄 Autofix (Beta)
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
Run ID: 6b6700ea-bcae-4442-b3d6-dbcee354674f
📒 Files selected for processing (11)
docs/admin_ui_key_visualizer_design.mddocs/design/2026_04_27_implemented_keyviz_cluster_fanout.mddocs/design/2026_04_27_proposed_keyviz_cluster_fanout.mddocs/design/2026_04_28_implemented_keyviz_adapter_labels.mddocs/design/2026_04_29_proposed_logical_backup.mddocs/design/2026_06_11_partial_hotspot_split_milestone3_automation.mddocs/design/2026_06_23_proposed_scaling_roadmap.mdinternal/admin/keyviz_fanout.gointernal/admin/keyviz_handler.gomain.goscripts/rolling-update.env.example
💤 Files with no reviewable changes (1)
- docs/design/2026_04_27_proposed_keyviz_cluster_fanout.md
|
対応しました。
検証:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cffe865b7a
ℹ️ 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".
|
|
||
| ### 4.5 Time alignment | ||
|
|
||
| All nodes use `keyvizStep` (default 1 s), but the shipped Phase 2-C |
There was a problem hiding this comment.
The implemented default is keyviz.DefaultStep = 60 * time.Second and --keyvizStep uses that default, so documenting the shipped Phase 2-C default as 1 s gives operators the wrong sampling-window size and understates how far exact, unaligned peer columns can drift under the default configuration.
Useful? React with 👍 / 👎.
| Admin -- "gRPC: Distribution, RaftAdmin, Admin.KeyViz" --> Node1 | ||
| Admin -- "gRPC" --> Node2 | ||
| Admin -- "gRPC" --> Node3 | ||
| Browser -- "HTTP/JSON + WebSocket" --> Node1HTTP |
There was a problem hiding this comment.
This diagram now describes the shipped embedded KeyViz path, but the embedded admin router only registers the KeyViz matrix/hotkeys HTTP endpoints, and a repo-wide search shows no /api/stream or WebSocket handler in internal/admin. Leaving WebSocket on the implemented Node HTTP path promises a live-streaming API that clients cannot actually use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cffe865b7a
ℹ️ 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".
| Under stable leadership, exactly one node (the current leader for | ||
| the route's group) records writes for a given | ||
| `(bucketID, raftGroupID, leaderTerm, column)`; other nodes report | ||
| 0. Zero-valued contributions are ignored for write conflict | ||
| purposes, so follower zeros against the leader's non-zero value do | ||
| not perturb the result or raise `conflicts[j]`. |
There was a problem hiding this comment.
When clients send writes through followers or through multiple ingress nodes in the same Raft term, this stable-leadership contract is not what the server records: ShardedCoordinator.groupMutations calls observeMutation before ShardRouter.Commit forwards, and the leader-side Internal.Forward path commits directly without re-entering that observation hook. Those ingress-local write samples share the same (raftGroupID, leaderTerm), so the documented max/dedupe rule undercounts normal traffic and may mark conflicts even without a leadership flip; either document the ingress-observed limitation or change observation/merge semantics before calling this exact.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e51f140a5b
ℹ️ 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".
| ### 9.1 Cluster-wide fan-out | ||
|
|
||
| Because writes are recorded by Raft leaders and follower-local reads are recorded by the followers that serve them (§5.1), pointing the admin binary at a single node produces a **partial heatmap**. To give operators a complete view by default, the admin binary runs in **fan-out mode**: | ||
| Because writes are recorded by Raft leaders and follower-local reads are recorded by the followers that serve them (§5.1), a single node's local admin view produces a **partial heatmap**. Phase 2-C ships a narrower embedded fan-out path inside each server process: |
There was a problem hiding this comment.
In follower-ingress or multi-ingress deployments this section still tells operators that KeyViz writes are leader-recorded, but the actual write hook runs in ShardedCoordinator.groupMutations before forwarding (kv/sharded_coordinator.go:2048), and the leader-side Internal.Forward path commits directly without re-observing (adapter/internal.go:65). That makes the merge rules below look like exact leader-only accounting even though the implemented fan-out doc now correctly says write cells are ingress-observed samples; please align this parent section so operators do not misinterpret normal same-term conflicts or under-counts as Raft anomalies.
Useful? React with 👍 / 👎.
| trip the cap. The cap firing surfaces as a JSON-decode error | ||
| from the peer (the LimitReader returns EOF mid-stream), which | ||
| the aggregator records as `ok=false` with the decode error in | ||
| that node's status entry plus a `WARN`-level server log. No |
There was a problem hiding this comment.
This states that hitting the peer response cap is always reported as ok=false, but fetchPeerJSON uses a LimitReader(cap+1) and returns success after logging when a complete JSON object decodes before the extra/trailing bytes; TestKeyVizFanoutRunPeerNearCapSucceedsWithWarning pins that path as OK=true. Operators relying on this doc will assume every over-cap response is excluded from the merge, so the implemented-status doc should distinguish bounded-read warnings from mid-object truncation/decode failures.
Useful? React with 👍 / 👎.
| control plane being fan-out-aware (see | ||
| `2026_04_27_proposed_keyviz_cluster_fanout.md`). | ||
| `2026_04_27_implemented_keyviz_cluster_fanout.md`). |
There was a problem hiding this comment.
The new target is the implemented KeyViz HTTP fan-out design, but that design explicitly scopes itself to static --keyvizFanoutNodes and excludes GetClusterOverview/membership discovery, so it is not evidence that the Distribution control plane is fan-out-aware. Readers following this dependency for multi-cluster backup will land on the wrong contract; keep this reference on a future Distribution fan-out design or the parent section that still describes discovery semantics.
Useful? React with 👍 / 👎.
|
対応しました。
検証:
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
Current HEAD CI is green and the branch is MERGEABLE/CLEAN. @codex review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
Superseded by #1079, which merged the current implemented KeyViz fanout design into main. Closing this older duplicate branch to avoid carrying stale conflicts. |
1 participant
Summary
Design docs
Validation
Author: bootjp
Summary by CodeRabbit