keyspace: avoid loading all groups every second for node allocation#11026
keyspace: avoid loading all groups every second for node allocation#11026rleungx wants to merge 3 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughKeyspace-group allocation now uses leader-term-scoped, watcher-triggered reconciliation with cached memberships and revalidation. Cache writes use shared locked helpers. Etcd transaction reads now honor transaction contexts, with cancellation coverage added. ChangesKeyspace reconciliation
Context-aware etcd transactions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TSONodesWatcher
participant KeyspaceGroupWatcher
participant GroupManager
participant EtcdStorage
TSONodesWatcher->>GroupManager: notify TSO membership change
KeyspaceGroupWatcher->>GroupManager: apply keyspace-group membership change
GroupManager->>EtcdStorage: load candidate group
EtcdStorage-->>GroupManager: return persisted membership
GroupManager->>GroupManager: revalidate active reconciliation term
GroupManager->>EtcdStorage: save updated membership
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
labels
![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
🧹 Nitpick comments (1)
pkg/keyspace/tso_keyspace_group.go (1)
151-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
errors.Isfor the sentinel check.
err != errs.ErrKeyspaceGroupExistsonly works whilesaveKeyspaceGroupsreturns the sentinel unwrapped. It silently breaks if the error is ever wrapped (Wrap/FastGenByArgs).♻️ Suggested change
- if err != nil && err != errs.ErrKeyspaceGroupExists { + if err != nil && !errors.Is(err, errs.ErrKeyspaceGroupExists) { return err }As per coding guidelines: "Use
errors.Is/Asfor sentinel error checks".🤖 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 `@pkg/keyspace/tso_keyspace_group.go` around lines 151 - 153, Update the sentinel comparison in saveKeyspaceGroups to use errors.Is, preserving the existing behavior of returning errors other than ErrKeyspaceGroupExists while recognizing wrapped instances of that sentinel.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 `@pkg/storage/kv/kv_test.go`:
- Around line 49-58: Update testCanceledTxn to invoke txn.Load using the
already-canceled context before or alongside txn.Save, and assert the load
returns an error. Keep the existing transaction cancellation and
post-transaction key assertions unchanged so the test covers both txn.Load
cancellation and commit behavior.
---
Nitpick comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 151-153: Update the sentinel comparison in saveKeyspaceGroups to
use errors.Is, preserving the existing behavior of returning errors other than
ErrKeyspaceGroupExists while recognizing wrapped instances of that sentinel.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2683cc6-76ac-41a1-a3b1-fd0ffda8e79f
📒 Files selected for processing (6)
pkg/keyspace/keyspace.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/storage/kv/etcd_kv.gopkg/storage/kv/kv_test.gopkg/utils/etcdutil/etcdutil.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/keyspace/tso_keyspace_group_test.go`:
- Around line 238-262: Defer cancelOldLeader immediately after creating
oldLeaderCtx in the reconcile-term test, ensuring cleanup occurs on every exit
path including t.Fatal failures. Remove the trailing cancelOldLeader call after
the notification assertions, while preserving the existing deferred
cancelNewLeader cleanup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7af3fe4a-6d9e-4db9-883e-70509f0bb122
📒 Files selected for processing (6)
pkg/keyspace/keyspace.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/storage/kv/etcd_kv.gopkg/storage/kv/kv_test.gopkg/utils/etcdutil/etcdutil.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/storage/kv/kv_test.go
- pkg/storage/kv/etcd_kv.go
- pkg/keyspace/keyspace.go
- pkg/utils/etcdutil/etcdutil.go
- pkg/keyspace/tso_keyspace_group.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/keyspace/tso_keyspace_group_test.go (1)
526-528: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDefer
manager.Close()to prevent resource leaks.The
KeyspaceGroupManageris created but itsClosemethod is never deferred in this sub-benchmark. Omitting this cleanup can leak background goroutines and contexts across the benchmark run.🧹 Proposed fix
b.Run("watch-membership-only", func(b *testing.B) { manager := NewKeyspaceGroupManager(ctx, store, nil) + defer manager.Close() manager.groupWatcherTerm = 1🤖 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 `@pkg/keyspace/tso_keyspace_group_test.go` around lines 526 - 528, Defer cleanup for the KeyspaceGroupManager created in the “watch-membership-only” benchmark by calling manager.Close() immediately after NewKeyspaceGroupManager returns, ensuring its goroutines and contexts are released when the sub-benchmark exits.
🤖 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.
Outside diff comments:
In `@pkg/keyspace/tso_keyspace_group_test.go`:
- Around line 526-528: Defer cleanup for the KeyspaceGroupManager created in the
“watch-membership-only” benchmark by calling manager.Close() immediately after
NewKeyspaceGroupManager returns, ensuring its goroutines and contexts are
released when the sub-benchmark exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7860217-ea8e-4e80-a694-61d4807b767a
📒 Files selected for processing (3)
pkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/storage/kv/kv_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/storage/kv/kv_test.go
- pkg/keyspace/tso_keyspace_group.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #11027
The API leader periodically loaded every keyspace group from etcd to repair TSO node membership. With millions of keyspaces, this creates recurring etcd reads, JSON decoding, allocation, and GC pressure, and can affect availability.
What is changed and how does it work?
Original TiDB Cloud issue: https://github.com/tidbcloud/pd-cse/issues/559
Companion TiDB Cloud PR: https://github.com/tidbcloud/pd-cse/pull/567
Check List
Tests
Benchmark: the healthy event-driven path performs 0 etcd read bytes/op; membership-only watch processing does not decode the keyspace list.
Code changes
Side effects
Release note
Summary by CodeRabbit