tools: add show region by keyspace-id or table-id#9850
Open
rleungx wants to merge 4 commits intotikv:masterfrom
Open
tools: add show region by keyspace-id or table-id#9850rleungx wants to merge 4 commits intotikv:masterfrom
rleungx wants to merge 4 commits intotikv:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9850 +/- ##
==========================================
+ Coverage 78.88% 78.99% +0.10%
==========================================
Files 530 532 +2
Lines 71548 71998 +450
==========================================
+ Hits 56439 56873 +434
- Misses 11092 11094 +2
- Partials 4017 4031 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
JmPotato
reviewed
Oct 22, 2025
Comment on lines
+422
to
+428
| // NewRegionsByKeyspaceTableIDCommand returns regions with keyspace and tableID subcommand of regionCmd. | ||
| func NewRegionsByKeyspaceTableIDCommand() *cobra.Command { | ||
| r := &cobra.Command{ | ||
| Use: `keyspace-id <keyspace_id> [table-id <table_id>] [<limit>] [--jq="<query string>"]`, | ||
| Short: "show regions for keyspace or table", | ||
| Run: showRegionsByKeyspaceTableIDCommandFunc, | ||
| } |
Member
There was a problem hiding this comment.
Should we make this command more general? That is, make the keyspace ID an optional parameter, so that for both the legacy architecture and the TiDB X architecture, we can use this command to query table regions with or without a keyspace ID.
Member
Author
There was a problem hiding this comment.
After considering this, the PR primarily focuses on the keyspace. We can add an option parameter to the region command separately.
Contributor
Reuse the existing region keyspace command instead of adding a parallel entry point for table-scoped lookups. Add request-shape tests so the keyspace and table range queries stay aligned with the current CLI behavior. Signed-off-by: Ryan Leung <rleungx@gmail.com>
📝 WalkthroughWalkthroughExtended Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "pd-ctl CLI"
participant Cmd as "region command"
participant Helper as "makeTableRangeInKeyspace"
participant HTTP as "HTTP client (dialClient)"
participant PD as "PD server / API"
CLI->>Cmd: invoke `region keyspace id ...`
Cmd->>Cmd: parse args, validate keyspaceID
alt table-id provided
Cmd->>Helper: compute startKey/endKey (keyspace BE + table codec)
Helper-->>Cmd: startKey, endKey
Cmd->>HTTP: GET /pd/api/v1/regions/key?key=...&end_key=...(&limit=...)
else no table-id
Cmd->>HTTP: GET /pd/api/v1/regions/keyspace/id/<id>(?limit=...)
end
HTTP->>PD: send request
PD-->>HTTP: response JSON
HTTP-->>Cmd: return response
Cmd-->>CLI: print output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/pd-ctl/pdctl/command/region_command_test.go`:
- Around line 77-80: The test is double-escaping keys by calling url.QueryEscape
on startKey/endKey before placing them into url.Values (query) which will later
be encoded; update the test to mirror the production fix in
showRegionWithKeyspaceCommandFunc by removing url.QueryEscape and set the raw
startKey and endKey into query (i.e., use query.Set("key", string(startKey)) and
query.Set("end_key", string(endKey))) so that query.Encode performs the proper
escaping; ensure you update the region_command_test.go lines that create query
and set these values to stop pre-escaping.
In `@tools/pd-ctl/pdctl/command/region_command.go`:
- Around line 596-605: The function makeTableRangeInKeyspace currently drops the
high byte by using keyspaceIDBytes[1:], so keyspace IDs > 2^24-1 are silently
truncated; update makeTableRangeInKeyspace to validate the parsed keyspaceID
(from strconv.ParseUint) to ensure it fits in 3 bytes (keyspaceIDUint64 <=
0xFFFFFF) and return an error (or panic/handle per surrounding convention) when
it does not, or alternatively change the encoding logic to include all 4 bytes
consistently; locate the ParseUint, keyspaceIDBytes and keyPrefix uses in
makeTableRangeInKeyspace to implement the bounds check or encoding change.
🪄 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: d05b559a-3917-46ed-8dff-f00b4e7ab59b
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_command_test.go
Keep table range lookups aligned with url.Values encoding and reject keyspace IDs outside the supported 24-bit range. Update the command tests to match the request shape and cover the out-of-range keyspace case. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pd-ctl/pdctl/command/region_command_test.go (1)
53-55: PreparedialClientswapping for concurrent test execution.Currently, tests that mutate the package-global
dialClientdo not uset.Parallel(), so no race condition exists. However, to make these tests safe ift.Parallel()is adopted in the future, add a test-local mutex helper witht.Cleanupfor restoration.♻️ Suggested refactor
import ( "bytes" "encoding/binary" "io" "net/http" "net/url" + "sync" "strings" "testing" @@ type captureRegionRoundTripper struct { @@ } + +var dialClientSwapMu sync.Mutex + +func withDialClient(t *testing.T, rt http.RoundTripper) { + t.Helper() + dialClientSwapMu.Lock() + oldClient := dialClient + dialClient = &http.Client{Transport: rt} + t.Cleanup(func() { + dialClient = oldClient + dialClientSwapMu.Unlock() + }) +} @@ func TestRegionKeyspaceIDPath(t *testing.T) { re := require.New(t) rt := &captureRegionRoundTripper{body: `{"ok":true}`} - oldClient := dialClient - dialClient = &http.Client{Transport: rt} - defer func() { dialClient = oldClient }() + withDialClient(t, rt)Also applies to lines 73-75, 98-100, 118-120.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/region_command_test.go` around lines 53 - 55, The test mutates the package-global variable dialClient directly, which can race if t.Parallel() is later used; wrap the swap in a test-local mutex/restorer pattern: create a local sync.Mutex (or sync.RWMutex) and a helper function that acquires the mutex, saves the original dialClient, sets dialClient = &http.Client{Transport: rt}, and registers a t.Cleanup that restores the original value and releases the mutex; replace the direct swap in the region_command_test functions (the places that currently do oldClient := dialClient / dialClient = &http.Client{Transport: rt} / defer restore) with calls to this helper, and apply the same change to the other similar spots referenced in the comment so each test uses the test-local mutex and t.Cleanup for safe concurrent execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/region_command_test.go`:
- Around line 53-55: The test mutates the package-global variable dialClient
directly, which can race if t.Parallel() is later used; wrap the swap in a
test-local mutex/restorer pattern: create a local sync.Mutex (or sync.RWMutex)
and a helper function that acquires the mutex, saves the original dialClient,
sets dialClient = &http.Client{Transport: rt}, and registers a t.Cleanup that
restores the original value and releases the mutex; replace the direct swap in
the region_command_test functions (the places that currently do oldClient :=
dialClient / dialClient = &http.Client{Transport: rt} / defer restore) with
calls to this helper, and apply the same change to the other similar spots
referenced in the comment so each test uses the test-local mutex and t.Cleanup
for safe concurrent execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae434cf8-ad1f-4ca8-a168-11e9676c19be
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_command_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pd-ctl/pdctl/command/region_command.go
Align the new region keyspace helpers with the repository static checks by fixing import ordering and using named return values. This keeps the CI-only cleanup together with the helper logic and its direct test coverage. Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pd-ctl/pdctl/command/region_command_test.go (1)
211-218: Consider extracting the nested appends for readability.The helper duplicates production logic intentionally for independent verification. The nested
appendchains are safe but dense. Consider extracting the common key construction pattern if you prefer improved readability:func expectedTableRangeInKeyspace(keyspaceID uint32, tableID int64) (startKey, endKey []byte) { keyspaceIDBytes := make([]byte, 4) binary.BigEndian.PutUint32(keyspaceIDBytes, keyspaceID) keyPrefix := append([]byte{'x'}, keyspaceIDBytes[1:]...) buildKey := func(tID int64) []byte { tableKey := append([]byte{'t'}, codec.EncodeInt(nil, tID)...) return codec.EncodeBytes(nil, append(keyPrefix, tableKey...)) } return buildKey(tableID), buildKey(tableID + 1) }This is optional—the current implementation matches the production code structure exactly, which has its own merit for traceability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/region_command_test.go` around lines 211 - 218, The nested append chains in expectedTableRangeInKeyspace produce correct keys but are dense; refactor by extracting the common key construction into a small helper/closure (e.g., buildKey inside expectedTableRangeInKeyspace) that takes tableID (tID) and returns codec.EncodeBytes(nil, append(keyPrefix, append([]byte{'t'}, codec.EncodeInt(nil, tID)...)...)); then call buildKey(tableID) and buildKey(tableID+1) to produce startKey and endKey, keeping keyPrefix and the binary.BigEndian conversion intact for traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/region_command_test.go`:
- Around line 211-218: The nested append chains in expectedTableRangeInKeyspace
produce correct keys but are dense; refactor by extracting the common key
construction into a small helper/closure (e.g., buildKey inside
expectedTableRangeInKeyspace) that takes tableID (tID) and returns
codec.EncodeBytes(nil, append(keyPrefix, append([]byte{'t'},
codec.EncodeInt(nil, tID)...)...)); then call buildKey(tableID) and
buildKey(tableID+1) to produce startKey and endKey, keeping keyPrefix and the
binary.BigEndian conversion intact for traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c548d65-2195-4011-bc26-5e7148ccb66f
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_command_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pd-ctl/pdctl/command/region_command.go
Member
Author
|
/retest |
bufferflies
reviewed
Apr 7, 2026
| @@ -0,0 +1,219 @@ | |||
| // Copyright 2025 TiKV Project Authors. | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // Copyright 2025 TiKV Project Authors. | |
| // Copyright 2026 TiKV Project Authors. |
| r.AddCommand(&cobra.Command{ | ||
| Use: "id <keyspace_id> <limit>", | ||
| Use: "id <keyspace_id> [table-id <table_id>] [<limit>]", | ||
| Short: "show region information for the given keyspace id", |
Contributor
There was a problem hiding this comment.
should also modify the short description.
Clarify that table-id is an optional filter on keyspace lookup and align the new test file header year with the current review update. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/pd-ctl/pdctl/command/region_command_test.go (2)
211-219:expectedTableRangeInKeyspacemirrors production logic too closely.Because this helper duplicates the production algorithm, regressions can slip through if both evolve together. Prefer at least one independent golden-case assertion for the encoded query payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/region_command_test.go` around lines 211 - 219, The test helper expectedTableRangeInKeyspace duplicates production encoding logic, risking coupled regressions; change the tests to include an independent golden-case assertion for the encoded query payload rather than relying solely on expectedTableRangeInKeyspace: keep the helper for convenience (function expectedTableRangeInKeyspace) but add at least one test that asserts the exact expected encoded bytes (a hard-coded golden value) for a representative keyspaceID/tableID pair, and compare the actual EncodeBytes/EncodeInt output against that golden byte slice to detect divergence between production code and the test helper.
50-139: Consider extracting repeated command/transport setup into a helper.The same setup/teardown and stdout wiring is repeated across multiple tests; a shared helper would reduce noise and future update cost.
♻️ Proposed refactor
+func runRegionWithKeyspaceCommand(t *testing.T, args []string, rt *captureRegionRoundTripper) string { + t.Helper() + oldClient := dialClient + dialClient = &http.Client{Transport: rt} + t.Cleanup(func() { dialClient = oldClient }) + + cmd := NewRegionWithKeyspaceCommand() + cmd.PersistentFlags().String("pd", "http://mock-pd:2379", "") + cmd.SetArgs(args) + + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + require.NoError(t, cmd.Execute()) + return out.String() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/region_command_test.go` around lines 50 - 139, Several tests (TestRegionKeyspaceIDPath, TestRegionKeyspaceIDTableIDPath, TestRegionKeyspaceIDPathWithLimit, TestRegionKeyspaceIDTableIDPathWithLimit) repeat the same dialClient/transport setup and stdout wiring; extract a helper, e.g. newRegionTestHarness(rtBody string) (or setupRegionCmd(rt *captureRegionRoundTripper)) that creates the captureRegionRoundTripper, swaps dialClient to &http.Client{Transport: rt}, returns the constructed *cobra.Command from NewRegionWithKeyspaceCommand(), the transport instance, an output buffer, and a cleanup func to restore dialClient; update each test to call the helper, set args on the returned cmd, call cmd.Execute(), and defer the cleanup to remove the duplicated setup/teardown and output wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/region_command_test.go`:
- Around line 211-219: The test helper expectedTableRangeInKeyspace duplicates
production encoding logic, risking coupled regressions; change the tests to
include an independent golden-case assertion for the encoded query payload
rather than relying solely on expectedTableRangeInKeyspace: keep the helper for
convenience (function expectedTableRangeInKeyspace) but add at least one test
that asserts the exact expected encoded bytes (a hard-coded golden value) for a
representative keyspaceID/tableID pair, and compare the actual
EncodeBytes/EncodeInt output against that golden byte slice to detect divergence
between production code and the test helper.
- Around line 50-139: Several tests (TestRegionKeyspaceIDPath,
TestRegionKeyspaceIDTableIDPath, TestRegionKeyspaceIDPathWithLimit,
TestRegionKeyspaceIDTableIDPathWithLimit) repeat the same dialClient/transport
setup and stdout wiring; extract a helper, e.g. newRegionTestHarness(rtBody
string) (or setupRegionCmd(rt *captureRegionRoundTripper)) that creates the
captureRegionRoundTripper, swaps dialClient to &http.Client{Transport: rt},
returns the constructed *cobra.Command from NewRegionWithKeyspaceCommand(), the
transport instance, an output buffer, and a cleanup func to restore dialClient;
update each test to call the helper, set args on the returned cmd, call
cmd.Execute(), and defer the cleanup to remove the duplicated setup/teardown and
output wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9910f7a-672f-421a-b278-8e7acb2717bd
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_command_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pd-ctl/pdctl/command/region_command.go
Member
Author
|
/retest |
Member
Author
|
/retest |
bufferflies
approved these changes
Apr 14, 2026
Contributor
|
[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 |
Contributor
[LGTM Timeline notifier]Timeline:
|
Member
Author
|
/retest |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #9707, cp https://github.com/tidbcloud/pd-cse/pull/355, close #10531
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit
New Features
region keyspace idaccepts an optional table-id and a flexible limit, enforces stricter keyspace-ID validation with clearer error messages, and constructs either a table-specific key-range query or a limit-only query as appropriate.Tests