Skip to content

tools: isolate keyspace according to the given label #10121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
rleungx wants to merge 9 commits into
base: master
Choose a base branch
from
Open

tools: isolate keyspace according to the given label #10121

rleungx wants to merge 9 commits into from
+719 −17

Conversation

@rleungx
Copy link
Member

@rleungx rleungx commented Dec 26, 2025 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #10122

What is changed and how does it work?

Check List

Tests

  • Unit test

Release note

None.

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 26, 2025
@codecov
Copy link

codecov bot commented Dec 26, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 70.90909% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.39%. Comparing base (205352f) to head (92a9008).
⚠️ Report is 1 commits behind head on master.

❌ Your patch check has failed because the patch coverage (70.90%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #10121      +/-   ##
==========================================
+ Coverage   78.38%   78.39%   +0.01%     
==========================================
  Files         518      518              
  Lines       69476    69584     +108     
==========================================
+ Hits        54456    54551      +95     
- Misses      11066    11073       +7     
- Partials     3954     3960       +6
Flag Coverage Δ
unittests 78.39% <70.90%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rleungx rleungx requested review from bufferflies and lhy1024 January 4, 2026 07:39
@rleungx
add tiflash test
cc03035 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
Copy link
Member Author

rleungx commented Jan 5, 2026

/retest

lhy1024
lhy1024 reviewed Jan 5, 2026
View reviewed changes
tools/pd-ctl/pdctl/command/keyspace_command.go Outdated
}

// Generate key ranges for the keyspace
keyspaceID32 := uint32(keyspaceID)
Copy link
Contributor

@lhy1024 lhy1024 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check whether keyspace id is valid?

tools/pd-ctl/pdctl/command/keyspace_command.go
GroupID: groupID,
ID: fmt.Sprintf("keyspace-%d-rule", keyspaceID),
Role: pd.Voter,
Count: 3,
Copy link
Contributor

@lhy1024 lhy1024 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the cluster is 5 replicas, what will happen?

Copy link
Member Author

@rleungx rleungx Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, maybe we can consider it later.

Copy link
Contributor

@lhy1024 lhy1024 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding a TODO for it?

tools/pd-ctl/pdctl/command/keyspace_command.go Outdated
keyspaceIDBytes := make([]byte, 4)
nextKeyspaceIDBytes := make([]byte, 4)
binary.BigEndian.PutUint32(keyspaceIDBytes, id)
binary.BigEndian.PutUint32(nextKeyspaceIDBytes, id+1)
Copy link
Contributor

@lhy1024 lhy1024 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it overflow?

Copy link
Member Author

@rleungx rleungx Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is the same as MakeRegionBound in keyspace/util.go

rleungx added 6 commits January 5, 2026 11:54
@rleungx
address the comment
f5ab2f2 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
clean up
183c948 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
clean up
3c219ca 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
clean up
15d0807 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
fix
8911105 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
fix
92a9008 
Signed-off-by: Ryan Leung <[email protected]>
@rleungx
Copy link
Member Author

rleungx commented Jan 5, 2026

/retest

1 similar comment
@rleungx
Copy link
Member Author

rleungx commented Jan 5, 2026

/retest

@rleungx rleungx requested a review from lhy1024 January 5, 2026 07:54
@rleungx
Copy link
Member Author

rleungx commented Jan 5, 2026

/retest-required

lhy1024
lhy1024 reviewed Jan 5, 2026
View reviewed changes
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rest LGTM. I am worried about hard code about placement rule.

tools/pd-ctl/pdctl/command/keyspace_command.go
groupID := fmt.Sprintf("keyspace-%d", keyspaceID)
bundle := &pd.GroupBundle{
ID: groupID,
Index: 100,
Copy link
Contributor

@lhy1024 lhy1024 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is index100 expected?

Copy link
Member Author

@rleungx rleungx Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't have any idea about this value.

@rleungx
add TODO
15a014d 
Signed-off-by: Ryan Leung <[email protected]>
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 5, 2026
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 5, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lhy1024

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 5, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-01-05 08:43:16.495879268 +0000 UTC m=+605352.314187700: ☑️ agreed by lhy1024.

@ti-chi-bot ti-chi-bot bot added the approved label Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhy1024 lhy1024 lhy1024 approved these changes

@bufferflies bufferflies Awaiting requested review from bufferflies

Assignees

No one assigned

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rleungx @lhy1024