Reserve dedicated TSO raft group#1064
Conversation
📝 WalkthroughWalkthroughTSO用Raft group ID 0を予約し、データルートやSQS FIFO割り当てから除外する検証を追加した。専用グループへのリーダー固定、全グループ操作、ランタイムディレクトリ配置、関連テストと設計文書を更新した。 Changes専用TSOグループ対応
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeConfig
participant configureCoordinatorTSO
participant ShardedCoordinator
participant TimestampGroupEngine
participant DataGroupEngine
RuntimeConfig->>configureCoordinatorTSO: groups構成を渡す
configureCoordinatorTSO->>ShardedCoordinator: group 0と全データグループを設定
ShardedCoordinator->>TimestampGroupEngine: group 0のリーダー状態を確認
TimestampGroupEngine-->>ShardedCoordinator: リーダー判定結果
ShardedCoordinator->>DataGroupEngine: 設定済みグループへlease・ブロードキャストを実行
Possibly related PRs
Suggested reviewers: 🚥 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 |
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 (296fe03):
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 (296fe03):
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. |
![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 reserves and bootstraps a dedicated Raft group (ID 0) for the Centralized Timestamp Oracle (TSO). It updates the ShardedCoordinator to pin timestamp issuance leadership to this dedicated group when configured, ensures that shard ranges cannot route user data to group 0, and updates the design documentation and tests accordingly. There are no review comments, so we have no additional 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: 296fe03024
ℹ️ 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".
|
Addressed the current-head dedicated TSO group findings. Changes:
Caller audit:
Verification:
|
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 (333d12a):
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 333d12a022
ℹ️ 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".
| and `--tsoEnabled` pins timestamp issuance leadership to group 0 when that | ||
| group is present. Existing `--raftGroups` / `--raftGroupPeers` bootstrap the |
There was a problem hiding this comment.
This says --tsoEnabled pins timestamp leadership to group 0 when that group is present, but this commit deliberately leaves WithTimestampGroup(0) unwired in configureCoordinatorTSO, so LocalTSOAllocator still accepts any locally-led data shard via IsTimestampLeader. For deployments adding reserved group 0, the doc now describes a stricter timestamp authority than the code actually enforces, which can mislead rollout planning and incident diagnosis until the redirect path exists.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func validateSQSFifoPartitionMapNoDedicatedTSOGroup(partitionMap map[string]sqsFifoQueueRouting) error { | ||
| for queue, routing := range partitionMap { |
There was a problem hiding this comment.
When a config contains more than one queue that routes a partition to group 0, this new validation returns whichever queue Go's randomized map iteration visits first. The existing validateSQSFifoPartitionMap sorts queue names specifically to keep operator diagnostics and golden-test assertions deterministic, so multi-error configs can become flaky here; iterate over sorted queue keys before checking the reserved group.
Useful? React with 👍 / 👎.
|
@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 (e776b16):
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e776b16e62
ℹ️ 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".
| if c.timestampGroupConfigured { | ||
| return isLeaderEngine(engineForGroup(c.groups[c.timestampGroup])) | ||
| } | ||
| for _, g := range c.groups { | ||
| if isLeaderEngine(engineForGroup(g)) { |
There was a problem hiding this comment.
When --tsoEnabled is used in the current bridge path, configureCoordinatorTSO deliberately does not call WithTimestampGroup, so LocalTSOAllocator.IsLeader falls through to this method's unpinned scan. Because that scan still considers every entry in c.groups, a process that leads only reserved group 0 now reports itself as a timestamp leader and can serve timestamps before the TSO-leader redirect path exists, adding group 0 as another independent issuer despite the bridge being intended to use locally led data shards only. Filter the fallback to the configured data groups (or otherwise skip group 0) when timestampGroupConfigured is false.
Useful? React with 👍 / 👎.
| if g.id == dedicatedTSORaftGroupID { | ||
| continue |
There was a problem hiding this comment.
When an existing single-group deployment adds only the reserved group 0 (for example changing from group 1 to groups 0+1), this new acceptance path makes the config valid but runtimeConfig.multi is still computed from len(groups) > 1; buildShardGroups then passes multi=true to groupDataDir, moving group 1 from the existing base/raftID store to base/raftID/group-1. That makes the upgraded data group start from an empty/new directory instead of its existing Raft/MVCC state. Derive multi from data groups or otherwise keep group 1 on the legacy path when group 0 is the only added group.
Useful? React with 👍 / 👎.
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 (438fa9d):
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 対応しました。
Caller audit:
検証:
|
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
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 (7c12731):
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
kv/sharded_coordinator.go (2)
478-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueスライスのコピーに
slices.Cloneを使用するGo 1.21以降では、
appendを用いたコピーの代わりにslices.Cloneを使用することで、スライスの複製をより意図が明確で簡潔に記述できます。♻️ 修正案
func (c *ShardedCoordinator) WithAllShardGroups(groupIDs ...uint64) *ShardedCoordinator { - c.allShardGroupIDs = append([]uint64(nil), groupIDs...) + c.allShardGroupIDs = slices.Clone(groupIDs) slices.Sort(c.allShardGroupIDs) c.allShardGroupIDs = slices.Compact(c.allShardGroupIDs) c.allShardGroupsConfigured = true🤖 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 478 - 484, WithAllShardGroups should use slices.Clone(groupIDs) instead of append([]uint64(nil), groupIDs...) when copying the input slice, while preserving the existing sorting, compaction, configuration flag, and return behavior.
1794-1810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueマップキーの抽出とソートを簡略化する
Go 1.23以降で利用可能な
slices.Sortedとmaps.Keysを活用することで、手動で行っているキーの抽出とソート処理を1行で簡潔に記述できます。※ 適用する場合は、importブロックに
"maps"パッケージの追加が必要です。♻️ 修正案
func (c *ShardedCoordinator) ownedAllShardGroups() ([]*ShardGroup, error) { - gids := make([]uint64, 0, len(c.groups)) - for gid := range c.groups { - gids = append(gids, gid) - } - slices.Sort(gids) + gids := slices.Sorted(maps.Keys(c.groups)) out := make([]*ShardGroup, 0, len(gids)) for _, gid := range gids {🤖 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 1794 - 1810, In ownedAllShardGroups, replace the manual gid collection and sorting with maps.Keys followed by slices.Sorted, adding the maps import if needed. Preserve the existing iteration order, nil-group ErrLeaderNotFound handling, and returned shard groups.multiraft_runtime_test.go (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
require.NoDirExistsの使用を検討してください。
os.Statとos.IsNotExistを用いた冗長なエラーチェックの代わりに、Testifyのrequire.NoDirExistsを使用することで、アサーションの意図を明確にしつつ記述を簡潔にできます。♻️ 提案するリファクタリング
- if _, err := os.Stat(filepath.Join(legacyDir, "group-1")); !os.IsNotExist(err) { - require.NoError(t, err) - t.Fatalf("group 1 should stay in legacy dir, but group-1 dir exists") - } + require.NoDirExists(t, filepath.Join(legacyDir, "group-1"), "group 1 should stay in legacy dir")🤖 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 `@multiraft_runtime_test.go` around lines 75 - 78, Replace the manual os.Stat/os.IsNotExist check for filepath.Join(legacyDir, "group-1") with Testify’s require.NoDirExists assertion, preserving the test’s expectation that the directory does not exist and removing the redundant failure handling.shard_config_test.go (1)
308-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueテーブル駆動テスト (Table-driven tests) の活用を検討してください。
コーディングガイドラインにて「Goのユニットテストではテーブル駆動テストを優先すること」が定められています。以下の箇所において、複数のケースが独立したテスト関数や個別の
t.Runとして列挙されています。テストケースを構造体のスライス（テーブル）としてまとめ、ループ処理で実行するようにリファクタリングすることで、今後のケース追加や保守が容易になります。
shard_config_test.go#L308-L331:TestBuildSQSFifoPartitionMap...の2つのテスト関数を、1つのテーブル駆動テストに統合してください。multiraft_runtime_test.go#L23-L30:TestGroupDataDir内の個別のt.Run呼び出しを、スライスを用いたテーブル駆動テストの形式へ変更してください。🤖 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 `@shard_config_test.go` around lines 308 - 331, Refactor the two SQS FIFO partition-map tests in shard_config_test.go (lines 308-331) into one table-driven TestBuildSQSFifoPartitionMap test, preserving each case’s input and assertions while running them through subtests. Also update TestGroupDataDir in multiraft_runtime_test.go (lines 23-30) to define its cases in a slice and iterate over them with t.Run; both sites require direct changes.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.
Nitpick comments:
In `@kv/sharded_coordinator.go`:
- Around line 478-484: WithAllShardGroups should use slices.Clone(groupIDs)
instead of append([]uint64(nil), groupIDs...) when copying the input slice,
while preserving the existing sorting, compaction, configuration flag, and
return behavior.
- Around line 1794-1810: In ownedAllShardGroups, replace the manual gid
collection and sorting with maps.Keys followed by slices.Sorted, adding the maps
import if needed. Preserve the existing iteration order, nil-group
ErrLeaderNotFound handling, and returned shard groups.
In `@multiraft_runtime_test.go`:
- Around line 75-78: Replace the manual os.Stat/os.IsNotExist check for
filepath.Join(legacyDir, "group-1") with Testify’s require.NoDirExists
assertion, preserving the test’s expectation that the directory does not exist
and removing the redundant failure handling.
In `@shard_config_test.go`:
- Around line 308-331: Refactor the two SQS FIFO partition-map tests in
shard_config_test.go (lines 308-331) into one table-driven
TestBuildSQSFifoPartitionMap test, preserving each case’s input and assertions
while running them through subtests. Also update TestGroupDataDir in
multiraft_runtime_test.go (lines 23-30) to define its cases in a slice and
iterate over them with t.Run; both sites require direct changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 446a4707-ef50-47fe-9996-a7612e07637b
📒 Files selected for processing (11)
docs/design/2026_04_16_partial_centralized_tso.mdkv/sharded_coordinator.gokv/sharded_coordinator_leader_test.gokv/sharded_coordinator_txn_test.gokv/tso_test.gomain.gomain_encryption_startup_guard.gomultiraft_runtime.gomultiraft_runtime_test.goshard_config.goshard_config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shard_config.go
- docs/design/2026_04_16_partial_centralized_tso.md
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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
Summary
--tsoEnabledtimestamp issuance leadership to group 0 when that group is configured, while preserving the existing any-led-shard bridge when it is absent.Tests
go test . -run 'TestDefaultGroupID|TestParseRuntimeConfigAllowsDedicatedTSOGroupWithoutDataRoute|TestValidateShardRanges'go test ./kv -run 'TestShardedCoordinatorReportsAnyShardAsTimestampLeader|TestShardedCoordinatorUsesDedicatedTimestampGroupLeader|TestLocalTSOAllocator|TestBatchAllocator'go test ./store -count=1go test ./...currently fails inadapter:Test_grpc_transactionandTest_consistency_satisfy_write_after_read_sequencetime out.Test_grpc_transactionalso times out on a cleanorigin/mainworktree under the same local conditions, so this is not introduced by this branch.Summary by CodeRabbit