Mark KeyViz fanout design implemented#1079
Conversation
|
@codex review |
|
Warning Review limit reached
Next review available in: 18 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 Cluster Fan-outの実装済み仕様として、Raft識別子によるマージ、競合情報、wire形式を文書化し、関連参照を実装版へ統一した。整合性テストの反復回数も定数化して更新した。 ChangesKeyVizファンアウト実装仕様
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
![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' and updates its implementation section to reflect the completed Phase 2-C MVP. It also renames all references to this design document across other design files, source code comments, and configuration templates to match the new filename. I have no feedback to provide as there are no review comments.
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
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 307 to 309 in ce334d1
Stop documenting missing catalog-divergence warnings
Now that this document is promoted to implemented, this wire contract is misleading for clusters whose peers disagree on a bucket's Start/End: FanoutNodeStatus only serializes node, ok, and error, and repo search shows no catalog_divergence emitter, so clients will never receive the documented warnings array. Either the warning field/emitter needs to exist, or this implemented doc should not promise it.
elastickv/docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
Lines 249 to 251 in ce334d1
Stop claiming fan-out aligns skewed columns
For nodes whose flush ticks are offset by sub-step jitter, this implemented doc promises the aggregator buckets timestamps to the keyvizStep boundary, but the current merge still uses exact ColumnUnixMs values (mergeKeyVizMatrices calls unionColumns, which keys by the raw timestamp, and Flush stamps s.now()). Those peers will produce adjacent columns instead of one merged window, so the implemented design should either describe the actual exact-timestamp behavior or add the alignment.
ℹ️ 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-C+ remains future work: extend the matrix rows with | ||
| `raftGroupID` and `leaderTerm`, replace §4.2's max-merge with the | ||
| canonical `(bucketID, raftGroupID, leaderTerm, windowStart)` merge, | ||
| and promote `conflict` from per-row to per-cell. That follow-up is | ||
| explicitly outside this Phase 2-C implemented slice. |
There was a problem hiding this comment.
This new implementation-status paragraph is already stale for the current tree: Phase 2-C+ is not future work here. KeyVizRow already carries raft_group_ids, leader_terms, and per-cell conflicts, mergeKeyVizMatrices performs the (bucketID, raftGroupID, leaderTerm, column) write merge, and docs/design/2026_05_25_implemented_keyviz_per_cell_conflict.md records that PR-3d is implemented. Leaving this as a future follow-up will send future reviewers/operators to plan an already-shipped wire shape instead of the implemented 2-C+ docs.
Useful? React with 👍 / 👎.
| - `internal/admin/keyviz_fanout_test.go`, | ||
| `internal/admin/keyviz_handler_test.go`, and SPA tests cover the | ||
| server and UI behavior while preserving the default single-node |
There was a problem hiding this comment.
This overstates the current test evidence: web/admin/src/pages/KeyViz.test.ts only exercises parseBucketID, and web/admin/src/api/client.test.ts does not cover the fanout shape, degraded banner, conflict hatching, or cluster-view counter. Since this status block is now the implemented evidence for the feature, claiming SPA tests cover the UI behavior can cause future reviewers to skip adding the missing coverage or misjudge regressions in those paths.
Useful? React with 👍 / 👎.
|
Addressed the latest review findings and CI blocker.
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de739473d4
ℹ️ 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-C+ is also implemented: matrix rows carry `raft_group_ids`, | ||
| `leader_terms`, and per-cell `conflicts`; write merging uses the | ||
| canonical `(bucketID, raftGroupID, leaderTerm, column)` identity and | ||
| falls back to the legacy max-merge when a peer omits the group/term |
There was a problem hiding this comment.
When this promoted implemented design is used as the source of truth, this new Phase 2-C+ status note conflicts with the still-unchanged scope and merge-rule sections above: §2.2 still says raftGroupID/leaderTerm are not built and §4.2/§4.3 still describe write counters as max-merge, while the code now uses the group/term dedupe described here. Please update the earlier contract sections instead of appending a contradictory status note, or future work/tests can follow the obsolete max-merge behavior.
Useful? React with 👍 / 👎.
| The field is **always present** on every row in the response — even | ||
| when fan-out is disabled (single-node mode), `conflict` defaults to |
There was a problem hiding this comment.
For the normal no-conflict path described here, KeyVizRow.Conflict is tagged json:"conflict,omitempty" in internal/admin/keyviz_handler.go, so false values are omitted rather than emitted. Clients following this implemented wire contract could incorrectly depend on "conflict" in row being stable in single-node or clean fan-out responses; please either remove the “always present” guarantee or change the struct tag/encoder behavior to match it.
Useful? React with 👍 / 👎.
| divergence the operator should investigate; the current wire | ||
| contract does not emit a structured catalog-divergence warning, so | ||
| operators must correlate the row identity with peer status and the | ||
| route catalog when investigating that condition. |
There was a problem hiding this comment.
For a symmetric list where node B's --keyvizFanoutNodes starts with node A, this implemented doc still directs operators to reason from the lowest-indexed configured node, but KeyVizFanout.Run prepends the local matrix before peer matrices and mergeRowInto copies Start/End only when a bucket is first seen, so node B will publish B's row metadata whenever it has the row. Operators investigating catalog divergence from this doc can inspect the wrong node/order; please state the local-first/then-peer-list behavior or change the merge to match the documented ordering.
Useful? React with 👍 / 👎.
|
@codex review 対応しました。
検証:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md (1)
2-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
implemented化後の将来形を残さないでください。Line 2で実装済みに昇格していますが、§6には「同じPRまたは直後のfollow-upで実装可能」という提案段階の文章が残っています。実装済みの挙動を記録する過去形へ更新してください。
As per coding guidelines, 「
*_implemented_*.mdは全マイルストーン実装済みの as-built record として保持する」必要があります。🤖 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/design/2026_04_27_implemented_keyviz_cluster_fanout.md` at line 2, Update §6 in the implemented design document to describe the completed behavior in the past tense, removing future-state or proposal wording such as implementation in the same PR or a follow-up. Preserve the document as an as-built record with all milestones represented as implemented.Source: Coding guidelines
🤖 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/design/2026_04_27_implemented_keyviz_cluster_fanout.md`:
- Around line 205-208: Update the documentation section describing aggregation
across distinct leaderTerm values to explicitly define the cell’s wire identity:
each leader_terms[j] must contain the maximum-contributing term selected by
resolveWrite(), while legacy fallback cases may use 0. Clarify that this
identity is separate from summing all per-term contributions.
- Around line 226-229: DNS aliases for the local node can bypass
isSelfFanoutNode and cause local rows to be merged twice. Update the
KeyVizFanout.Run/--keyvizFanoutNodes design contract to explicitly mark
self-referencing DNS aliases as unsupported, instructing callers not to
configure them unless self-node normalization and deduplication are implemented.
---
Outside diff comments:
In `@docs/design/2026_04_27_implemented_keyviz_cluster_fanout.md`:
- Line 2: Update §6 in the implemented design document to describe the completed
behavior in the past tense, removing future-state or proposal wording such as
implementation in the same PR or a follow-up. Preserve the document as an
as-built record with all milestones represented as implemented.
🪄 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: 4ddbe630-1baa-4304-b038-9551adc42678
📒 Files selected for processing (3)
adapter/grpc_test.goadapter/test_util.godocs/design/2026_04_27_implemented_keyviz_cluster_fanout.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5cb41ce51
ℹ️ 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-C+ is also implemented: matrix rows carry `raft_group_ids`, | ||
| `leader_terms`, and per-cell `conflicts`; write merging uses the | ||
| canonical `(bucketID, raftGroupID, leaderTerm, column)` identity and | ||
| falls back to the legacy max-merge when a peer omits the group/term | ||
| identity. |
There was a problem hiding this comment.
This section now states the Phase 2-C+ group/term merge is implemented, but the Five-lens checklist immediately below still describes the old conservative max-merge behavior and says the §9.1 canonical rule is only a future contract. Since this file is now the implemented reference, readers of the checklist get the opposite consistency contract for leader-term deployments; please update §8 to describe the current group/term sum plus legacy fallback.
Useful? React with 👍 / 👎.
| non-zero source omits group/term identity, the cell falls back | ||
| to the Phase 2-C legacy max-merge so mixed-version peers never |
There was a problem hiding this comment.
This fallback is the mixed-version behavior, but the rollout note earlier in this design still says an old node that omits new fields is treated as the merge-rule identity. In actual mergeKeyVizMatrices, missing raft_group_ids/leader_terms on a non-zero cell forces the legacy max-merge path, so mixed-version clusters may not get canonical per-term summation; please align the rollout note to avoid promising exact group/term dedupe during upgrades.
Useful? React with 👍 / 👎.
| write hot path (`observeMutation`, `kv/sharded_coordinator.go:1841-1846`), | ||
| with proposed extensions for cluster fan-out | ||
| (`2026_04_27_proposed_keyviz_cluster_fanout.md`), | ||
| (`2026_04_27_implemented_keyviz_cluster_fanout.md`), |
There was a problem hiding this comment.
After updating this reference to the implemented fan-out design, the surrounding roadmap still categorizes cluster fan-out together with implemented subrange sampling, hot-key top-K, and per-cell conflict as “proposed extensions.” That leaves the scaling roadmap contradicting the linked documents' status and can mislead readers trying to distinguish shipped KeyViz signals from future work; please split shipped items from genuinely proposed extensions or update the wording.
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: 0b825d735c
ℹ️ 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".
| _ "google.golang.org/grpc/health" | ||
| ) | ||
|
|
||
| const grpcSequenceIterations = 256 |
There was a problem hiding this comment.
This new shared constant cuts both gRPC sequence consistency loops from 9,999 iterations to 256. In the normal CI/full-test path, these are the only long-running write-after-read and transactional put/get/delete sequence checks in this file, so regressions that only surface after many repeated operations or during mid-loop Raft churn are now much less likely to be exercised. Please keep the previous iteration count for the full test, or gate the shorter count behind an explicit short/fast mode.
Useful? React with 👍 / 👎.
|
|
||
| - Phase 2-C (current): `(bucketID, raftGroupID, leaderTerm, | ||
| windowStart)` per design `2026_04_27_proposed_keyviz_cluster_fanout.md` | ||
| windowStart)` per design `2026_04_27_implemented_keyviz_cluster_fanout.md` |
There was a problem hiding this comment.
This updated reference now points readers at the implemented fan-out design, whose §4 says writes use (bucketID, raftGroupID, leaderTerm, column) dedupe/sum with a legacy fallback, but this same section still says below that “writes still max-with-conflict” and only the bucketID shape changes. Anyone following the adapter-label design for fan-out interactions would preserve the old max-merge semantics and miss the shipped per-term summation/fallback behavior, so the surrounding merge-rule text should be updated with the link.
Useful? React with 👍 / 👎.
| milliseconds — i.e. `time.Duration(--keyvizStep) / time.Millisecond`. | ||
| The wire form already carries timestamps as `column_unix_ms` so | ||
| millisecond-quantum arithmetic falls out naturally. | ||
| All nodes use `keyvizStep` (default 1 s), but the implemented |
There was a problem hiding this comment.
This as-built section still says the keyvizStep default is 1 second, but the shipped default is keyviz.DefaultStep = 60 * time.Second and the --keyvizStep flag uses that value. Operators relying on the implemented design will expect 1-second columns and may misread retention/window sizing or fan-out column alignment; please update this to the actual 60-second default or avoid hard-coding it here.
Useful? React with 👍 / 👎.
|
@codex review 対応しました。
検証:
|
|
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 review refresh requested. Latest head ae59fa7 already reconciles the KeyViz fanout merge contract:
CI is green on the latest head. @codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
1 participant
Author: bootjp
Summary
Validation
Summary by CodeRabbit