tso: keyspace group auto splitting#10932
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Hi @ystaticy. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
ti-chi-bot
Bot
added
contribution
|
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:
📝 WalkthroughWalkthroughAdds periodic automatic keyspace-group splitting to ChangesKeyspace-Group Auto-Split Patrol
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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)
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 |
ti-chi-bot
Bot
added
the
size/XL
ystaticy
changed the title
![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/tso_keyspace_group.go`:
- Around line 249-255: The doPatrolKeyspaceGroupSizeForAutoSplit method performs
storage reads/writes but lacks access to the caller's context, making
cancellation handling implicit. Add ctx context.Context as the first parameter
to the doPatrolKeyspaceGroupSizeForAutoSplit method definition, pass the ctx
from the calling site when invoking this method, and add a context cancellation
check at the beginning of the method to ensure the split work respects the
caller's context before proceeding with any storage operations.
- Around line 281-313: The code attempts to split keyspace groups without first
verifying they meet the minimum replica requirement, causing
SplitKeyspaceGroupByID to fail and return early, which blocks other eligible
groups from being auto-split. Add a precondition check after the existing
IsSplitting() and IsMerging() checks in the loop to skip groups that have fewer
than mcs.DefaultKeyspaceGroupReplicaCount members, similar to how
under-replicated groups are handled. This should check the group's member count
(likely available on the group object) against the minimum threshold and
continue to the next iteration if the group is under-replicated, preventing the
early return in the error handling of SplitKeyspaceGroupByID.
- Around line 270-274: Change the comparison operator from > to >= in the
condition checking nextID against mcs.MaxKeyspaceGroupCountInUse. The constant
MaxKeyspaceGroupCountInUse represents an exclusive upper bound for valid
keyspace group IDs (0-4095), and using >= ensures the check correctly prevents
assigning ID 4096, which would be rejected by the validation function
checkKeySpaceGroupID and exceed the bounds of the allocators and kgs array
declarations.
🪄 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: 9c1b5e1f-2b3c-40ae-827a-de8b7a11d5fb
📒 Files selected for processing (2)
pkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.go
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
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`:
- Line 963: The test assertions on lines 963 and 969 use an incorrect split
index calculation. Since the eligibleKeyspaces slice has
defaultKeyspaceCountSplitThreshold + 1 elements, the split index should be
(defaultKeyspaceCountSplitThreshold + 1) / 2 to match what the actual patrol
logic computes, not defaultKeyspaceCountSplitThreshold / 2. Update both
assertions to use the correct split index formula that accounts for the +1
additional element in the eligible keyspaces count.
🪄 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: 68453620-4523-4632-a3dc-8cde9ad493cd
📒 Files selected for processing (2)
pkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/tso_keyspace_group.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10932 +/- ##
==========================================
- Coverage 79.26% 79.19% -0.07%
==========================================
Files 540 541 +1
Lines 74954 75171 +217
==========================================
+ Hits 59412 59533 +121
- Misses 11371 11449 +78
- Partials 4171 4189 +18
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
|
@ystaticy: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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. |
|
/ok-to-test |
|
@ystaticy: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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. |
JmPotato
removed
the
needs-ok-to-test
Signed-off-by: ystaticy <y_static_y@sina.com>
|
@ystaticy: 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. |
|
[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:
|
| keyspacesToMove := make([]uint32, 0, count-splitIdx) | ||
| for i := splitIdx; i < count; i++ { | ||
| kid := group.Keyspaces[i] | ||
| if kid == constant.DefaultKeyspaceID { |
There was a problem hiding this comment.
In NextGen, the protected bootstrap keyspace is SystemKeyspaceID, not 0. If SystemKeyspaceID is included in this tail half, every patrol attempt fails and auto-split remains stuck.
| maxID = g.ID | ||
| } | ||
| } | ||
| nextID := maxID + 1 |
There was a problem hiding this comment.
This can make auto-splitting stop when a high-numbered group exists, even if lower group IDs are still free. For example, group 4095 blocks future auto-splits despite available IDs below it.
4 participants
What problem does this PR solve?
Issue Number: Close #10933
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests