tso: update keyspace group revision after initial load#10981
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughKeyspace group loading now preserves the highest observed ModRevision, startup failures in ChangesMod revision tracking and recovery readiness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Etcd
participant LoopWatcher
participant KeyspaceGroupManager
LoopWatcher->>Etcd: initial load (putFn per kv)
Etcd-->>LoopWatcher: kv.ModRevision
LoopWatcher->>LoopWatcher: track maxLoadedModRevision
LoopWatcher->>KeyspaceGroupManager: postEventsFn (no events, maxLoadedModRevision>0)
KeyspaceGroupManager->>KeyspaceGroupManager: SetModRevision(maxLoadedModRevision)
sequenceDiagram
participant TestSuite
participant TSONode
participant Etcd
participant KeyspaceGroupManager
TestSuite->>TSONode: restart node
TestSuite->>Etcd: poll keyspace-group KV
Etcd-->>TestSuite: KV with ModRevision
TestSuite->>KeyspaceGroupManager: check loaded group ID and revision
KeyspaceGroupManager-->>TestSuite: group ID, loaded revision
TestSuite->>TestSuite: confirm keyspace serving by group
TestSuite->>TSONode: proceed with recovery assertions
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 |
![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/utils/etcdutil/etcdutil_test.go (1)
470-496: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a tighter assertion for
GetLoadedRevision.
re.GreaterOrEqual(watcher.GetLoadedRevision(), targetRevision)only proves the loaded revision is not stale; it would not catch a bug where the-1offset is dropped or doubled, since either mistake still yields a value>= targetRevision. Since this test guards the exact race-condition fix targeted by this PR, a precise equality check (e.g., putting the key, then comparing against the exact revision returned by a subsequentGet/resp.Header.Revision) would give stronger regression protection.🤖 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/utils/etcdutil/etcdutil_test.go` around lines 470 - 496, The TestGetLoadedRevision assertion is too loose and won’t catch an incorrect revision offset calculation. Update the test in loopWatcherTestSuite to verify the exact loaded revision from NewLoopWatcher/GetLoadedRevision by comparing against the expected revision from the etcd operations used in the setup, rather than only using GreaterOrEqual. Keep the check focused on the loaded revision behavior so it fails if the -1 offset logic is wrong.
🤖 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/utils/etcdutil/etcdutil.go`:
- Around line 497-499: The batched load in the etcd watcher setup is mixing
revisions because each paginated Get in load() can observe a newer
Header.Revision, so the watcher may start from a point that skips writes between
pages. Update load() and the surrounding watcher initialization in etcdutil to
capture a single snapshot revision up front, reuse that revision for every batch
fetch, and have the final loadedRevision/watchStartRevision logic derive from
that fixed revision rather than each page’s response.
---
Nitpick comments:
In `@pkg/utils/etcdutil/etcdutil_test.go`:
- Around line 470-496: The TestGetLoadedRevision assertion is too loose and
won’t catch an incorrect revision offset calculation. Update the test in
loopWatcherTestSuite to verify the exact loaded revision from
NewLoopWatcher/GetLoadedRevision by comparing against the expected revision from
the etcd operations used in the setup, rather than only using GreaterOrEqual.
Keep the check focused on the loaded revision behavior so it fails if the -1
offset logic is wrong.
🪄 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: a1ed1963-6417-49a9-9731-ed548e16c7cc
📒 Files selected for processing (3)
pkg/tso/keyspace_group_manager.gopkg/utils/etcdutil/etcdutil.gopkg/utils/etcdutil/etcdutil_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10981 +/- ##
==========================================
+ Coverage 79.22% 79.26% +0.03%
==========================================
Files 541 541
Lines 75965 76042 +77
==========================================
+ Hits 60187 60274 +87
- Misses 11531 11532 +1
+ Partials 4247 4236 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-testserver-run-deadlock
branch
from
65b522a to
4777a56
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
2 participants
What problem does this PR solve?
Issue Number: Close #10551
TestUpdateMemberWhenRecoverycan still fail after a TSO node restart. The restarted TSO server loads keyspace group metadata from etcd, but its local keyspace group mod revision can remain 0 after the initial load. When an existing client asks with a newer service-discovery mod revision, the server rejects the request as stale even though it already has the keyspace group metadata.What is changed and how does it work?
Check List
Tests
Test scripts
Release note
Summary by CodeRabbit