Complete standalone hotspot split automation#1104
Conversation
…' into design/hotspot-split-m3-auto-scheduler
…r' into design/hotspot-split-m3-auto-scheduler # Conflicts: # proto/distribution.pb.go
…' into design/hotspot-split-m3-auto-scheduler
📝 WalkthroughWalkthrough自動分割の検出器・スケジューラ・ランタイム制御を追加し、KeyViz のホットキー観測、カタログ監視、分配サーバー、管理 gRPC と接続しました。Top-K 隔離、証拠フェンス、Prometheus 指標、E2E テスト、設計文書も更新されています。 ChangesAutosplit 検出とスケジューリング
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminServer
participant RuntimeSwitch
participant Scheduler
AdminClient->>AdminServer: SetAutoSplitEnabled(enabled)
AdminServer->>RuntimeSwitch: SetEnabled(enabled)
AdminServer-->>AdminClient: Enabled()
Scheduler->>RuntimeSwitch: KillSwitch()
RuntimeSwitch-->>Scheduler: enabled state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (48d83f0):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (8b22ccf):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
![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 implements the standalone automatic hotspot range split scheduler (autosplit) for same-group splits, integrating it with the existing SplitRange and KeyViz sampler. It introduces a durable SplitAtHLC route lineage field to reconstruct cooldowns on leadership changes, updates the route catalog codec to v2 with backward compatibility, and adds a transaction commit timestamp patching mechanism (CommitTSValueOffset). A critical issue was identified in the scheduler's execution loop where sequential split decisions are executed with a stale catalog version, which will cause subsequent splits in the same cycle to fail due to optimistic concurrency control (OCC) mismatches. Updating the catalog version dynamically after each successful split is recommended to resolve this.
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.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a18b63d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (00e9a82):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![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: 00e9a82fb8
ℹ️ 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".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4817c25):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Updated the scheduler follow-up on latest head. Addressed current findings around demo sampling, sampler route descriptor re-registration, and catalog-key leadership gating. Local checks: go test ./distribution/autosplit -count=1; go test . -count=1; go test ./cmd/server -count=1; go test ./kv -count=1; go test ./... -run '^$'; go test ./... -timeout=20m; golangci-lint --config=.golangci.yaml run --fix; git diff --check.\n\n@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4817c25fdb
ℹ️ 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".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (9f3f8a9):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Current-head evidence for Requirement audit:
Current-head fixes for review roots:
Verification:
Commit signature: Good EDDSA signature; author and committer are |
|
@codex review |
|
Superseded by the verified current-head evidence comment: #1104 (comment) |
bootjp
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f3f8a93c5
ℹ️ 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".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (3bd6a97):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
…r' into design/hotspot-split-m3-auto-scheduler # Conflicts: # Makefile # adapter/admin_grpc.go # proto/admin.pb.go # proto/service.pb.go
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (bf05e1d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Current head bf05e1d resolves the three latest review findings and is merged with current main through dependency PR #1097. Validation:
The previous full local go test run only failed the pre-existing long-running Redis stream latency test after its HLC physical ceiling expired under local load; current-head GitHub test and Jepsen checks are the authoritative full-suite gates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf05e1dd96
ℹ️ 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".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (e023ed4):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Current HEAD: Resolved all four current P2 findings and merged current main:
Caller/semantic audit:
Validation after merging current main:
@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: 3
🧹 Nitpick comments (2)
main_autosplit_e2e_test.go (2)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTickの戻り値を検証せずに破棄している
kill switch を無効化する直前のこの
Tick呼び出しは結果を_で捨てており、Scheduled/Failedが想定通りゼロであることをアサートしていない。ここで意図しない追加split(または部分失敗)が発生してもテストは検知できない。提案: 結果を検証する
- _, err = leaderScheduler.Tick(context.Background(), base.Add(2*time.Minute+time.Second)) - require.NoError(t, err) + settled, err := leaderScheduler.Tick(context.Background(), base.Add(2*time.Minute+time.Second)) + require.NoError(t, err) + require.Zero(t, settled.Scheduled) + require.Zero(t, settled.Failed)🤖 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 `@main_autosplit_e2e_test.go` around lines 49 - 50, Update the Tick call in the autosplit test to retain its result and assert that both Scheduled and Failed are zero before disabling the kill switch. Keep the existing error assertion and use the returned Tick result to detect unintended splits or partial failures.
90-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winconfidence re-earning後の最終splitでクラスタ全体への伝播確認が欠けている
47行目・66行目の分割成功時は
waitForCatalogVersion(t, nodes, ...)で全ノードへの伝播を確認しているが、96-99行目のreearnedsplit ではcurrentCatalogVersionを対象ノードのみでチェックしており、他ノードへの反映は未検証。3ノードE2Eテストの目的(クラスタ全体の一貫性)に対して非対称であり、他の成功パスと揃えるべき。また、8分/9分の書き込みでは
notYet.Detector.Decisionsが空になり、10分でようやく成功する(初回splitは2ウィンドウで成立)という挙動の違いは、リーダー移譲後の信頼再構築に必要なウィンドウ数の意図が読み手に伝わりにくい。簡潔なコメントを添えると可読性が上がる。提案: 最終splitの伝播確認を追加
reearned, err := targetScheduler.Tick(context.Background(), base.Add(10*time.Minute)) require.NoError(t, err) require.Equal(t, 1, reearned.Scheduled) - require.Greater(t, currentCatalogVersion(t, nodes[targetIdx]), secondSplitVersion) + finalVersion := currentCatalogVersion(t, nodes[targetIdx]) + require.Greater(t, finalVersion, secondSplitVersion) + waitForCatalogVersion(t, nodes, finalVersion, waitTimeout, waitInterval)🤖 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 `@main_autosplit_e2e_test.go` around lines 90 - 99, 最終のreearned splitが対象ノードだけでなくクラスタ全体へ伝播したことを検証する。targetScheduler の10分時点の成功後、currentCatalogVersion の単一ノード確認を waitForCatalogVersion(t, nodes, ...) に置き換え、既存の成功パスと揃える。8分・9分では判定に必要なウィンドウが不足していることが10分で成功する意図を示す簡潔なコメントを notYet の確認付近に追加する。
🤖 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 `@kv/coordinator.go`:
- Around line 268-271: Synchronize updates to sampler, samplerRouteID, and
samplerRouteResolve with the lock-free reads performed by the observe* methods.
Update WithSamplerRouteResolver and the corresponding coordinator fields so all
three values are replaced atomically under the same synchronization mechanism,
preserving consistent routing during range-split reconfiguration.
In `@kv/sharded_coordinator.go`:
- Around line 1707-1723: NewShardedCoordinatorで受け取ったgroups
mapをそのまま保持せず、初期化時に独立したコピーを作成してc.groupsへ格納してください。GroupLeadershipとLeadershipForKeyはそのコピーを参照し、初期化後の外部map変更によるdata
raceを防いでください。
In `@main_autosplit.go`:
- Around line 109-146: Update setupDistributionWatcherAndAutoSplit so the
disabled --autoSplit path returns a true nil interface for
autosplit.RuntimeSwitch rather than a typed-nil *autosplit.RuntimeSwitch.
Preserve the existing runtime creation and KillSwitch setup when cfg.Enabled is
true, while ensuring callers such as the AutoSplitRuntime assignment can detect
the feature as unavailable.
---
Nitpick comments:
In `@main_autosplit_e2e_test.go`:
- Around line 49-50: Update the Tick call in the autosplit test to retain its
result and assert that both Scheduled and Failed are zero before disabling the
kill switch. Keep the existing error assertion and use the returned Tick result
to detect unintended splits or partial failures.
- Around line 90-99: 最終のreearned
splitが対象ノードだけでなくクラスタ全体へ伝播したことを検証する。targetScheduler
の10分時点の成功後、currentCatalogVersion の単一ノード確認を waitForCatalogVersion(t, nodes, ...)
に置き換え、既存の成功パスと揃える。8分・9分では判定に必要なウィンドウが不足していることが10分で成功する意図を示す簡潔なコメントを notYet
の確認付近に追加する。
🪄 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: 5e29aa3d-154e-4f04-b63f-44891679ed6a
⛔ Files ignored due to path filters (2)
proto/admin.pb.gois excluded by!**/*.pb.goproto/admin_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
adapter/admin_grpc.goadapter/admin_grpc_autosplit_test.goadapter/distribution_server.goadapter/distribution_server_test.gocmd/server/demo.gocmd/server/demo_test.godistribution/autosplit/detector.godistribution/autosplit/detector_rapid_test.godistribution/autosplit/detector_test.godistribution/autosplit/metrics.godistribution/autosplit/metrics_test.godistribution/autosplit/runtime_switch.godistribution/autosplit/runtime_switch_test.godistribution/autosplit/scheduler.godistribution/autosplit/scheduler_test.godistribution/watcher.godistribution/watcher_test.godocs/design/2026_02_18_partial_hotspot_shard_split.mddocs/design/2026_06_11_implemented_hotspot_split_milestone3_automation.mddocs/design/2026_06_11_implemented_leader_balance_scheduler.mddocs/design/2026_06_12_proposed_scaling_roadmap.mddocs/design/2026_06_23_proposed_scaling_roadmap.mdkeyviz/flusher.gokeyviz/flusher_test.gokeyviz/hot_keys.gokeyviz/hot_keys_test.gokeyviz/ring_buffer.gokeyviz/sampler.gokv/coordinator.gokv/coordinator_dispatch_test.gokv/raft_engine.gokv/sharded_coordinator.gomain.gomain_autosplit.gomain_autosplit_e2e_test.gomain_autosplit_test.gomain_bootstrap_e2e_test.goproto/admin.proto
| leaseObserver LeaseReadObserver | ||
| sampler keyviz.Sampler | ||
| samplerRouteID uint64 | ||
| samplerRouteResolve func(key []byte) (uint64, bool) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# WithSampler / WithSamplerRouteResolver の呼び出し箇所を確認
rg -nP -C4 '\.With(Sampler|SamplerRouteResolver)\s*\(' --type=goRepository: bootjp/elastickv
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files 'kv/*' | sed -n '1,120p'
printf '\n## coordinator outline\n'
ast-grep outline kv/coordinator.go --view expanded || true
printf '\n## sampler-related identifiers\n'
rg -n --context 3 'WithSampler|WithSamplerRouteResolver|samplerRouteID|samplerRouteResolve|sampler\s+keyviz\.Sampler' kv/coordinator.go kv -g '*.go' || trueRepository: bootjp/elastickv
Length of output: 25072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,360p' kv/coordinator.go | cat -nRepository: bootjp/elastickv
Length of output: 17276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('kv/coordinator.go').read_text()
for needle in ['WithSampler', 'WithSamplerRouteResolver', 'samplerRouteID', 'samplerRouteResolve', 'observe']:
print(f'\n## {needle}')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-4)
end = min(len(text.splitlines()), i+6)
for j in range(start, end+1):
print(f'{j}: {text.splitlines()[j-1]}')
print('---')
PYRepository: bootjp/elastickv
Length of output: 27458
sampler 配線の更新経路を同期するか、更新不可と明示してください。
WithSamplerRouteResolver は range split 後の再配線用途があり、observe* 側は sampler / samplerRouteID / samplerRouteResolve を無ロックで読んでいます。リクエスト処理中に差し替える前提ならデータレースになるので、3 フィールドを安全に一括更新できる形にするか、起動前のみ更新可だと明文化してください。
🤖 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 `@kv/coordinator.go` around lines 268 - 271, Synchronize updates to sampler,
samplerRouteID, and samplerRouteResolve with the lock-free reads performed by
the observe* methods. Update WithSamplerRouteResolver and the corresponding
coordinator fields so all three values are replaced atomically under the same
synchronization mechanism, preserving consistent routing during range-split
reconfiguration.
| // LeadershipForKey reports local leadership and Raft term for key's group. | ||
| func (c *ShardedCoordinator) LeadershipForKey(key []byte) (bool, uint64) { | ||
| g, ok := c.groupForKey(key) | ||
| if !ok { | ||
| return false, 0 | ||
| } | ||
| return leadershipForEngine(engineForGroup(g)) | ||
| } | ||
|
|
||
| // GroupLeadership reports local leadership and Raft term for groupID. | ||
| func (c *ShardedCoordinator) GroupLeadership(groupID uint64) (bool, uint64) { | ||
| g, ok := c.groups[groupID] | ||
| if !ok { | ||
| return false, 0 | ||
| } | ||
| return leadershipForEngine(engineForGroup(g)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# c.groups への書き込み・保護状況を確認
rg -nP -C3 '\bc\.groups\b' kv/sharded_coordinator.go
# groups フィールド定義と保護用ロックの有無
ast-grep run --pattern 'groups map[$_]$_' --lang go kv/sharded_coordinator.go
rg -nP -C2 'groups\s+map\[' kv/sharded_coordinator.goRepository: bootjp/elastickv
Length of output: 6077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# コンストラクタ周辺を確認
sed -n '655,705p' kv/sharded_coordinator.go
echo
echo '--- call sites ---'
rg -n "NewShardedCoordinator\(" -S .
echo
echo '--- any assignments to c.groups after construction ---'
rg -n "\bc\.groups\s*=" kv/sharded_coordinator.go
echo
echo '--- any writes via groups map in this file ---'
rg -n "\bc\.groups\[[^]]+\]\s*=" kv/sharded_coordinator.goRepository: bootjp/elastickv
Length of output: 11603
groups をコピーするか、不変である契約を明記してください。
NewShardedCoordinator は受け取った map をそのまま c.groups に保持しています。GroupLeadership / LeadershipForKey はバックグラウンド goroutine からロックなしで参照されるため、初期化後に外部から map を書き換えられると data race になります。
🤖 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 `@kv/sharded_coordinator.go` around lines 1707 - 1723,
NewShardedCoordinatorで受け取ったgroups
mapをそのまま保持せず、初期化時に独立したコピーを作成してc.groupsへ格納してください。GroupLeadershipとLeadershipForKeyはそのコピーを参照し、初期化後の外部map変更によるdata
raceを防いでください。
| func setupDistributionWatcherAndAutoSplit( | ||
| ctx context.Context, | ||
| eg *errgroup.Group, | ||
| catalog *distribution.CatalogStore, | ||
| engine *distribution.Engine, | ||
| distServer *adapter.DistributionServer, | ||
| coordinate *kv.ShardedCoordinator, | ||
| sampler *keyviz.MemSampler, | ||
| observer autosplit.Observer, | ||
| ) (*autosplit.RuntimeSwitch, error) { | ||
| cfg, err := configureRuntimeAutoSplit(coordinate) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cfg.Observer = observer | ||
| var runtime *autosplit.RuntimeSwitch | ||
| if cfg.Enabled { | ||
| runtime = autosplit.NewRuntimeSwitch(true) | ||
| cfg.KillSwitch = runtime.KillSwitch | ||
| } | ||
| var reconciler *autosplit.RouteReconciler | ||
| if sampler != nil { | ||
| reconciler = autosplit.NewRouteReconciler(sampler) | ||
| initialSnapshot, snapshotErr := catalog.Snapshot(ctx) | ||
| if snapshotErr != nil { | ||
| return nil, errors.Wrap(snapshotErr, "load initial catalog for keyviz reconciliation") | ||
| } | ||
| reconciler.Reconcile(initialSnapshot.Routes) | ||
| cfg.Reconciler = reconciler | ||
| } | ||
| eg.Go(func() error { | ||
| return runDistributionCatalogWatcher(ctx, catalog, engine, func(snapshot distribution.CatalogSnapshot) { | ||
| reconciler.Reconcile(snapshot.Routes) | ||
| }) | ||
| }) | ||
| startAutoSplitScheduler(ctx, eg, catalog, distServer, coordinate, sampler, cfg) | ||
| return runtime, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--autoSplit 無効時に SetAutoSplitEnabled が Unavailable を返さない（typed-nil 問題）
setupDistributionWatcherAndAutoSplit は無効時に nil の *autosplit.RuntimeSwitch を返します。これが main.go:611 で interface 型フィールド adapter.AutoSplitRuntime に代入されると、nil ポインタを包んだ「非 nil な interface」（typed-nil）になります。結果として main.go:2120 の if autoSplitRuntime != nil が true となり typed-nil な switch が登録され、AdminServer.SetAutoSplitEnabled は codes.Unavailable（未設定）ではなく成功（enabled=false）を返し、有効化操作も no-op になります。--autoSplit はデフォルト無効なので既定経路で発生します。keyviz サンプラが main.go:2113-2116 のコメント通り concrete 型で nil 判定して回避しているのと同じ罠です。
無効時に真の nil interface を返すよう修正するのが簡潔です。
🐛 Proposed fix
sampler *keyviz.MemSampler,
observer autosplit.Observer,
-) (*autosplit.RuntimeSwitch, error) {
+) (adapter.AutoSplitRuntime, error) {
cfg, err := configureRuntimeAutoSplit(coordinate)
if err != nil {
return nil, err
}
@@
startAutoSplitScheduler(ctx, eg, catalog, distServer, coordinate, sampler, cfg)
- return runtime, nil
+ if runtime == nil {
+ return nil, nil
+ }
+ return runtime, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func setupDistributionWatcherAndAutoSplit( | |
| ctx context.Context, | |
| eg *errgroup.Group, | |
| catalog *distribution.CatalogStore, | |
| engine *distribution.Engine, | |
| distServer *adapter.DistributionServer, | |
| coordinate *kv.ShardedCoordinator, | |
| sampler *keyviz.MemSampler, | |
| observer autosplit.Observer, | |
| ) (*autosplit.RuntimeSwitch, error) { | |
| cfg, err := configureRuntimeAutoSplit(coordinate) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cfg.Observer = observer | |
| var runtime *autosplit.RuntimeSwitch | |
| if cfg.Enabled { | |
| runtime = autosplit.NewRuntimeSwitch(true) | |
| cfg.KillSwitch = runtime.KillSwitch | |
| } | |
| var reconciler *autosplit.RouteReconciler | |
| if sampler != nil { | |
| reconciler = autosplit.NewRouteReconciler(sampler) | |
| initialSnapshot, snapshotErr := catalog.Snapshot(ctx) | |
| if snapshotErr != nil { | |
| return nil, errors.Wrap(snapshotErr, "load initial catalog for keyviz reconciliation") | |
| } | |
| reconciler.Reconcile(initialSnapshot.Routes) | |
| cfg.Reconciler = reconciler | |
| } | |
| eg.Go(func() error { | |
| return runDistributionCatalogWatcher(ctx, catalog, engine, func(snapshot distribution.CatalogSnapshot) { | |
| reconciler.Reconcile(snapshot.Routes) | |
| }) | |
| }) | |
| startAutoSplitScheduler(ctx, eg, catalog, distServer, coordinate, sampler, cfg) | |
| return runtime, nil | |
| } | |
| func setupDistributionWatcherAndAutoSplit( | |
| ctx context.Context, | |
| eg *errgroup.Group, | |
| catalog *distribution.CatalogStore, | |
| engine *distribution.Engine, | |
| distServer *adapter.DistributionServer, | |
| coordinate *kv.ShardedCoordinator, | |
| sampler *keyviz.MemSampler, | |
| observer autosplit.Observer, | |
| ) (adapter.AutoSplitRuntime, error) { | |
| cfg, err := configureRuntimeAutoSplit(coordinate) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cfg.Observer = observer | |
| var runtime *autosplit.RuntimeSwitch | |
| if cfg.Enabled { | |
| runtime = autosplit.NewRuntimeSwitch(true) | |
| cfg.KillSwitch = runtime.KillSwitch | |
| } | |
| var reconciler *autosplit.RouteReconciler | |
| if sampler != nil { | |
| reconciler = autosplit.NewRouteReconciler(sampler) | |
| initialSnapshot, snapshotErr := catalog.Snapshot(ctx) | |
| if snapshotErr != nil { | |
| return nil, errors.Wrap(snapshotErr, "load initial catalog for keyviz reconciliation") | |
| } | |
| reconciler.Reconcile(initialSnapshot.Routes) | |
| cfg.Reconciler = reconciler | |
| } | |
| eg.Go(func() error { | |
| return runDistributionCatalogWatcher(ctx, catalog, engine, func(snapshot distribution.CatalogSnapshot) { | |
| reconciler.Reconcile(snapshot.Routes) | |
| }) | |
| }) | |
| startAutoSplitScheduler(ctx, eg, catalog, distServer, coordinate, sampler, cfg) | |
| if runtime == nil { | |
| return nil, nil | |
| } | |
| return runtime, nil | |
| } |
🤖 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 `@main_autosplit.go` around lines 109 - 146, Update
setupDistributionWatcherAndAutoSplit so the disabled --autoSplit path returns a
true nil interface for autosplit.RuntimeSwitch rather than a typed-nil
*autosplit.RuntimeSwitch. Preserve the existing runtime creation and KillSwitch
setup when cfg.Enabled is true, while ensuring callers such as the
AutoSplitRuntime assignment can detect the feature as unavailable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e023ed4608
ℹ️ 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".
| cfg, | ||
| catalog, | ||
| autoSplitDistributionSplitter{server: distServer}, | ||
| sampler, |
There was a problem hiding this comment.
When clients send writes to followers, the sampler update happens on the ingress coordinator before forwarding (for example the sharded path observes during dispatch entry), while the leader-side Internal.Forward commit path does not update this node's sampler. Since this scheduler is only eligible on the catalog/shard leaders but consumes only the local sampler passed here, follower-ingress hotspots never reach the threshold on the node that is allowed to split. Please either observe forwarded commits on the leader or feed the scheduler merged/fan-in sampler data.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return | ||
| } | ||
| c.sampler.Observe(routeID, mut.Key, keyviz.OpWrite, len(mut.Value), keyviz.LabelLegacy) |
There was a problem hiding this comment.
In the single-group/demo path, when the resolver maps adapter storage keys such as S3 or filesystem keys through kv.RouteKey, this call still records the raw mut.Key into the sampler. The sampler's sub-bucket rows and Top-K entries are then in a different keyspace from the catalog route bounds that SplitRange normalizes before committing, so auto-split can choose raw-prefix boundaries or repeatedly retry a compound second split that normalizes back to the first boundary instead of isolating the hot route key. Pass the normalized route key to Observe (or have the resolver return it) for these paths.
Useful? React with 👍 / 👎.
| s.fenceUnprovenCatalogGap(prep.routes, prep.fences, newestWindowAt(windows)) | ||
| } | ||
| result := Evaluate(s.cfg.Detector, s.state, Input{ | ||
| Routes: prep.routes, |
There was a problem hiding this comment.
When GroupLeadership is configured, prep.routes has already been filtered to only the shard groups this node leads, but Evaluate uses the length of this slice for MaxRoutes admission. In a multi-group cluster where the catalog already has 1024 live routes but this node leads only a subset, the detector can still admit another split because the cap check sees only the subset; SplitRange has no independent max-route guard, so the global route cap can be exceeded. Pass the full snapshot route count into the admission check while still scoring only locally-led routes.
Useful? React with 👍 / 👎.
1 participant
Summary
Review roots addressed
Tests
Summary by CodeRabbit
新機能
バグ修正
ドキュメント