keyspace: support global safe point v2#10918
Conversation
ti-chi-bot
Bot
added
release-note-none
|
[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 |
|
@coderabbitai full review |
ti-chi-bot
Bot
added
the
size/L
✅ Action performedFull review finished. |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds keyspace global safe point v2 support: introduces a new config flag, storage path, and manager-side initialization and update flow. Server startup and reload paths now propagate keyspace config errors. Tests and constructor call sites were updated for the new error returns. ChangesKeyspace Global Safe Point V2
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 3
🤖 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/keyspace.go`:
- Around line 272-277: The UpdateConfig method in the Manager struct mutates
in-memory state by assigning to manager.config and calling
manager.mgm.updateGroups() before attempting the storage write in
SetGlobalSafePointV2(). If SetGlobalSafePointV2() fails, the method returns an
error but leaves the in-memory state partially modified. To fix this atomically,
reorder the operations so that SetGlobalSafePointV2() is called and checked for
errors first, then only if it succeeds should manager.config be assigned and
manager.mgm.updateGroups() be called. This ensures that if the storage operation
fails, no in-memory state is modified.
In `@server/config/config.go`:
- Around line 886-887: The EnableGlobalSafePointV2 field has a JSON tag with the
`,string` directive which forces it to accept only string-encoded boolean values
like "true" or "false", creating an inconsistency with other boolean fields like
WaitRegionSplit in the same struct that accept plain boolean literals. Remove
the `,string` directive from the json tag of EnableGlobalSafePointV2 to ensure
consistent wire-format handling of boolean types across all config fields in
KeyspaceConfig.
In `@server/server.go`:
- Around line 1219-1221: The UpdateConfig call on keyspaceManager occurs after a
successful Persist operation, creating a non-atomic sequence where Persist can
succeed but UpdateConfig can fail, leaving the caller with an error while the
config is already persisted. Either reorder the calls so UpdateConfig is invoked
before Persist to maintain atomicity, or implement a rollback mechanism that
undoes the Persist operation if UpdateConfig fails afterward.
🪄 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: aca86e34-bce1-47e7-a497-8669d2da0601
📒 Files selected for processing (9)
pkg/gc/gc_state_manager_test.gopkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/tso_keyspace_group_test.gopkg/storage/endpoint/keyspace.gopkg/utils/keypath/absolute_key_path.goserver/cluster/metering_test.goserver/config/config.goserver/server.go
Address review comments: - UpdateConfig rolls back in-memory config if SetGlobalSafePointV2 fails. - SetKeyspaceConfig reverts the persisted config when the post-persist keyspace manager update fails, keeping the operation atomic. - Drop the ,string json directive on enable-global-safe-point-v2 so it matches the other bool fields in KeyspaceConfig. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
@bufferflies: The following tests 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. |
2 participants
What problem does this PR solve?
Issue Number: Close #10886
CP from 78f39c6.
Original author: @ystaticy.
What is changed and how does it work?
Check List
Tests
Code changes
Related changes
Validation:
GOFLAGS=-buildvcs=false make gotest GOTEST_ARGS='./pkg/keyspace -run TestKeyspaceTestSuite -count=1 -timeout=5m'GOFLAGS=-buildvcs=false make gotest GOTEST_ARGS='./pkg/keyspace ./pkg/storage/endpoint ./server/config ./pkg/gc ./server/cluster -run "TestKeyspaceTestSuite|TestKeyspaceGroupTestSuite|TestNonExistent|TestCollectStorageSize|TestGCStateManager" -count=1 -timeout=5m'git diff --check && git diff --cached --checkGOFLAGS=-buildvcs=false make checkRelease note
Summary by CodeRabbit
New Features
enable-global-safe-point-v2configuration flag to activate Global Safe Point V2 support (default: disabled).Bug Fixes