tools: add show region by keyspace-id and table-id#10519
Closed
bufferflies wants to merge 4 commits intotikv:masterfrom
Closed
tools: add show region by keyspace-id and table-id#10519bufferflies wants to merge 4 commits intotikv:masterfrom
bufferflies wants to merge 4 commits intotikv:masterfrom
Conversation
ti-chi-bot
bot
added
release-note
Contributor
|
[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 |
40 tasks
Contributor
Author
|
@coderabbitai full review |
ti-chi-bot
bot
added
the
size/M
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (CLI)"
participant Cmd as "pd-ctl: regions cmd"
participant HTTP as "HTTP Client"
participant PD as "PD Server\n(/pd/api/v1/regions/key)"
User->>Cmd: invoke subcommand with args
Cmd->>Cmd: validate args (keyspace-id, optional table-id, limit)
Cmd->>Cmd: encode startKey / endKey (codec.EncodeBytes / EncodeInt)
Cmd->>HTTP: GET /pd/api/v1/regions/key?key=...&end_key=...(&limit=...)
HTTP->>PD: send HTTP request
PD-->>HTTP: respond (JSON)
HTTP-->>Cmd: return response
Cmd->>User: print response (or pipe through --jq)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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.go`:
- Around line 440-447: The code currently parses keyspaceID with
strconv.ParseInt and blindly splits it into 3 bytes (used to build
startKey/endKey), which allows negative or >24-bit values to be silently
truncated; change parsing/validation so keyspaceID is validated as an unsigned
24-bit value (0 <= keyspaceID <= 0xFFFFFF) before byte-splitting: either parse
with strconv.ParseUint or parse then check the int64 range, return a clear error
via cmd.Println if out of range, and only then construct startKey and endKey
using the validated keyspaceID variable (refer to the strconv.ParseInt call,
keyspaceID variable, and startKey/endKey construction).
- Around line 459-463: The endKey is incorrectly built by appending to startKey
(producing a key containing tableID then nextTableID), so change construction to
append to a fresh slice or to the original base prefix instead of startKey:
build endKey from the same base prefix used for startKey (e.g., use a new slice
or the base variable before codec.EncodeInt) and then call
codec.EncodeInt(endKey, nextTableID) so endKey becomes the correct prefix +
nextTableID; update the code paths around startKey, endKey, codec.EncodeInt,
tableID and nextTableID accordingly.
🪄 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: a7255fd5-89c5-4b45-b457-2ab2318bf701
📒 Files selected for processing (1)
tools/pd-ctl/pdctl/command/region_command.go
Signed-off-by: bufferflies <1045931706@qq.com>
Contributor
Author
|
@coderabbitai full review |
bufferflies
force-pushed
the
pr-merge/c93399a1-pdctl-region-keyspace-table
branch
from
4ef4351 to
c5405b3
Compare
✅ Actions performedFull review triggered. |
Signed-off-by: bufferflies <1045931706@qq.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
ti-chi-bot
bot
added
size/L
Signed-off-by: bufferflies <1045931706@qq.com>
Contributor
Author
|
Addressed the latest CodeRabbit findings in 0f0d54c:
Validation: @coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_test.go`:
- Around line 191-196: The test constructs endKey by appending to startKey which
masks a malformed range construction; change the test in region_test.go so
endKey is built from the intended prefix (e.g., construct the original prefix
bytes, encode the start int with codec.EncodeInt to form startKey, then
independently construct endKey by encoding the end int against that prefix
rather than appending to startKey) and assert against codec.EncodeBytes(...) and
mustQueryUnescape as before; also update showRegionsByKeyspaceTableIDCommandFunc
to build endKey from the prefix/namespace correctly (not by appending to the
computed startKey) so the command and test use the same correct construction
logic.
🪄 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: dc24ec5d-eb1e-4e50-8711-8d6d03dc8cad
📒 Files selected for processing (1)
tools/pd-ctl/pdctl/command/region_test.go
Comment on lines
+191
to
+196
| startKey := []byte{'x', 0, 0, 17, 't'} | ||
| startKey = codec.EncodeInt(startKey, 45) | ||
| endKey := append(startKey, 't') | ||
| endKey = codec.EncodeInt(endKey, 46) | ||
| re.Equal(string(codec.EncodeBytes(nil, startKey)), mustQueryUnescape(t, requestURL.Query().Get("key"))) | ||
| re.Equal(string(codec.EncodeBytes(nil, endKey)), mustQueryUnescape(t, requestURL.Query().Get("end_key"))) |
There was a problem hiding this comment.
endKey expectation is derived from startKey, which masks malformed range construction.
At Line 193, endKey := append(startKey, 't') bakes in the same bug pattern as the command implementation, so the test passes while validating an incorrect end_key shape.
🔧 Proposed test fix
startKey := []byte{'x', 0, 0, 17, 't'}
startKey = codec.EncodeInt(startKey, 45)
- endKey := append(startKey, 't')
+ endKey := []byte{'x', 0, 0, 17, 't'}
endKey = codec.EncodeInt(endKey, 46)
re.Equal(string(codec.EncodeBytes(nil, startKey)), mustQueryUnescape(t, requestURL.Query().Get("key")))
re.Equal(string(codec.EncodeBytes(nil, endKey)), mustQueryUnescape(t, requestURL.Query().Get("end_key")))Also update showRegionsByKeyspaceTableIDCommandFunc to construct endKey from the intended prefix instead of startKey, otherwise this test will still mirror the bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/pd-ctl/pdctl/command/region_test.go` around lines 191 - 196, The test
constructs endKey by appending to startKey which masks a malformed range
construction; change the test in region_test.go so endKey is built from the
intended prefix (e.g., construct the original prefix bytes, encode the start int
with codec.EncodeInt to form startKey, then independently construct endKey by
encoding the end int against that prefix rather than appending to startKey) and
assert against codec.EncodeBytes(...) and mustQueryUnescape as before; also
update showRegionsByKeyspaceTableIDCommandFunc to build endKey from the
prefix/namespace correctly (not by appending to the computed startKey) so the
command and test use the same correct construction logic.
🧩 Analysis chain🏁 Script executed: Length of output: 43593 🏁 Script executed: Length of output: 3038 🏁 Script executed: Length of output: 2413
🐛 Bug:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.go`:
- Around line 446-447: The code computes nextKeyspaceID := keyspaceID + 1 and
builds endKey from its low 24 bits, which wraps for the maximum 24-bit value
(keyspaceID == 16777215); change the logic so that when keyspaceID == 0xFFFFFF
you treat the end key as the exclusive end of space (e.g., nil or a suffix that
sorts after any valid key) instead of splitting nextKeyspaceID into 3 bytes;
update the construction of endKey in the region scan code (the variables
nextKeyspaceID and endKey in region_command.go) to check for this boundary and
generate a proper end sentinel, and add a regression test in
tools/pd-ctl/pdctl/command/region_test.go that asserts scans for
keyspace-id=16777215 produce the correct non-wrapping range.
🪄 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: 4be769f0-38b7-4ffc-91b4-512b684f7abc
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_test.go
Comment on lines
+446
to
+447
| nextKeyspaceID := keyspaceID + 1 | ||
| endKey := []byte{'x', byte(nextKeyspaceID >> 16), byte(nextKeyspaceID >> 8), byte(nextKeyspaceID)} |
There was a problem hiding this comment.
Handle the max keyspace-id boundary without 24-bit wraparound.
At Line 446 and Line 447, keyspace_id=16777215 (valid) makes nextKeyspaceID overflow the 24-bit keyspace byte range when split into 3 bytes, so endKey becomes smaller than startKey and the scan range is incorrect.
🔧 Proposed fix
startKey := []byte{'x', byte(keyspaceID >> 16), byte(keyspaceID >> 8), byte(keyspaceID)}
- nextKeyspaceID := keyspaceID + 1
- endKey := []byte{'x', byte(nextKeyspaceID >> 16), byte(nextKeyspaceID >> 8), byte(nextKeyspaceID)}
+ var endKey []byte
+ if keyspaceID == 0xFFFFFF {
+ // next prefix after all 'x'+24bit keyspace keys
+ endKey = []byte{'y'}
+ } else {
+ nextKeyspaceID := keyspaceID + 1
+ endKey = []byte{'x', byte(nextKeyspaceID >> 16), byte(nextKeyspaceID >> 8), byte(nextKeyspaceID)}
+ }Please also add a regression test for keyspace-id=16777215 in tools/pd-ctl/pdctl/command/region_test.go.
🤖 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.go` around lines 446 - 447, The
code computes nextKeyspaceID := keyspaceID + 1 and builds endKey from its low 24
bits, which wraps for the maximum 24-bit value (keyspaceID == 16777215); change
the logic so that when keyspaceID == 0xFFFFFF you treat the end key as the
exclusive end of space (e.g., nil or a suffix that sorts after any valid key)
instead of splitting nextKeyspaceID into 3 bytes; update the construction of
endKey in the region scan code (the variables nextKeyspaceID and endKey in
region_command.go) to check for this boundary and generate a proper end
sentinel, and add a regression test in tools/pd-ctl/pdctl/command/region_test.go
that asserts scans for keyspace-id=16777215 produce the correct non-wrapping
range.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/pd-ctl/pdctl/command/region_command.go (1)
445-447:⚠️ Potential issue | 🔴 CriticalHandle the max
keyspace-idboundary without 24-bit wraparound.When
keyspaceID = 0xFFFFFF(16777215),nextKeyspaceIDbecomes0x1000000. Truncating to 3 bytes producesendKey = []byte{'x', 0, 0, 0}, which is lexicographically smaller thanstartKey = []byte{'x', 0xFF, 0xFF, 0xFF}, resulting in an inverted/empty scan range.🔧 Proposed fix
startKey := []byte{'x', byte(keyspaceID >> 16), byte(keyspaceID >> 8), byte(keyspaceID)} - nextKeyspaceID := keyspaceID + 1 - endKey := []byte{'x', byte(nextKeyspaceID >> 16), byte(nextKeyspaceID >> 8), byte(nextKeyspaceID)} + var endKey []byte + if keyspaceID == 0xFFFFFF { + // Use next prefix after all 'x'+24bit keyspace keys + endKey = []byte{'y'} + } else { + nextKeyspaceID := keyspaceID + 1 + endKey = []byte{'x', byte(nextKeyspaceID >> 16), byte(nextKeyspaceID >> 8), byte(nextKeyspaceID)} + }🤖 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.go` around lines 445 - 447, The scan range wraps when keyspaceID == 0xFFFFFF because nextKeyspaceID overflows the 3-byte representation; update the logic that builds endKey to detect the max 24-bit value and use the next prefix byte instead of truncating: if keyspaceID == 0xFFFFFF set endKey = []byte{'x' + 1} (or the next top-level prefix used by your key schema), otherwise build endKey the same way using nextKeyspaceID; change the code that computes endKey (referencing keyspaceID, nextKeyspaceID, startKey, endKey) to implement this branch.
🧹 Nitpick comments (2)
tools/pd-ctl/pdctl/command/region_test.go (1)
200-226: Add regression test forkeyspace-id=16777215(max valid value).The existing test validates out-of-range rejection (16777216), but there's no test for the max valid keyspace ID (16777215). As noted in PR comments, when
keyspaceID = 0xFFFFFF, computingnextKeyspaceID = keyspaceID + 1overflows the 3-byte field, producing an incorrectendKey. Please add a test that verifiesend_keyuses a non-wrapping bound for this edge case.💡 Suggested test addition
func TestShowRegionsByKeyspaceTableIDCommandFuncWithMaxKeyspaceID(t *testing.T) { re := require.New(t) var requestURL *url.URL resp := `{"regions":[]}` oldClient := dialClient dialClient = &http.Client{ Transport: &captureRoundTripper{ roundTrip: func(req *http.Request) (*http.Response, error) { requestURL = req.URL return &http.Response{ StatusCode: http.StatusOK, Body: io.NopCloser(bytes.NewBufferString(resp)), }, nil }, }, } defer func() { dialClient = oldClient }() cmd := NewRegionsByKeyspaceTableIDCommand() cmd.Flags().String("pd", "http://mock-pd:2379", "") var out bytes.Buffer cmd.SetOut(&out) cmd.SetErr(&out) cmd.SetArgs([]string{"16777215"}) // max valid keyspace-id re.NoError(cmd.Execute()) re.NotNil(requestURL) expectedStartKey := codec.EncodeBytes(nil, []byte{'x', 0xFF, 0xFF, 0xFF}) // endKey should be 'y' (next prefix), not 'x',0,0,0 expectedEndKey := codec.EncodeBytes(nil, []byte{'y'}) re.Equal(string(expectedStartKey), mustQueryUnescape(t, requestURL.Query().Get("key"))) re.Equal(string(expectedEndKey), mustQueryUnescape(t, requestURL.Query().Get("end_key"))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/region_test.go` around lines 200 - 226, Add a regression test that verifies the max valid keyspace ID (16777215) does not wrap when computing end_key: implement TestShowRegionsByKeyspaceTableIDCommandFuncWithMaxKeyspaceID which replaces dialClient with a captureRoundTripper to capture the outgoing request URL, calls NewRegionsByKeyspaceTableIDCommand with arg "16777215", and asserts the captured request's "key" equals codec.EncodeBytes(nil, []byte{'x',0xFF,0xFF,0xFF}) and "end_key" equals codec.EncodeBytes(nil, []byte{'y'}) (use mustQueryUnescape to decode query values) and restore dialClient in a defer; this ensures the end_key uses the non-wrapping next prefix for the 3-byte keyspace field.tools/pd-ctl/pdctl/command/region_command.go (1)
467-474: Improve error message whentable-idvalue is missing.When a user runs
region keyspace-id 17 table-id(missing the table value),len(args) == 2causes the code to parse"table-id"as a limit, resulting in the confusing error"limit should be a number". Consider detecting this case and emitting a clearer message like"table-id requires a value".💡 Proposed improvement
if len(args) == 2 || len(args) == 4 { + // Guard against "keyspace-id <id> table-id" without value + if len(args) == 2 && args[1] == "table-id" { + cmd.Println("Error: ", "table-id requires a value") + return + } limit, err := strconv.Atoi(args[len(args)-1]) if err != nil { cmd.Println("Error: ", "limit should be a number") return } prefix += "&limit=" + strconv.Itoa(limit) }🤖 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.go` around lines 467 - 474, The code is parsing the last arg as a numeric limit in the len(args) == 2 || len(args) == 4 branch and misinterprets the literal "table-id" as a number; before calling strconv.Atoi on args[len(args)-1] (in region_command.go where args and limit are handled), add a guard that checks if args[len(args)-1] == "table-id" and if so print a clear error like "table-id requires a value" and return; otherwise proceed with strconv.Atoi and the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/pd-ctl/pdctl/command/region_command.go`:
- Around line 445-447: The scan range wraps when keyspaceID == 0xFFFFFF because
nextKeyspaceID overflows the 3-byte representation; update the logic that builds
endKey to detect the max 24-bit value and use the next prefix byte instead of
truncating: if keyspaceID == 0xFFFFFF set endKey = []byte{'x' + 1} (or the next
top-level prefix used by your key schema), otherwise build endKey the same way
using nextKeyspaceID; change the code that computes endKey (referencing
keyspaceID, nextKeyspaceID, startKey, endKey) to implement this branch.
---
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/region_command.go`:
- Around line 467-474: The code is parsing the last arg as a numeric limit in
the len(args) == 2 || len(args) == 4 branch and misinterprets the literal
"table-id" as a number; before calling strconv.Atoi on args[len(args)-1] (in
region_command.go where args and limit are handled), add a guard that checks if
args[len(args)-1] == "table-id" and if so print a clear error like "table-id
requires a value" and return; otherwise proceed with strconv.Atoi and the
existing behavior.
In `@tools/pd-ctl/pdctl/command/region_test.go`:
- Around line 200-226: Add a regression test that verifies the max valid
keyspace ID (16777215) does not wrap when computing end_key: implement
TestShowRegionsByKeyspaceTableIDCommandFuncWithMaxKeyspaceID which replaces
dialClient with a captureRoundTripper to capture the outgoing request URL, calls
NewRegionsByKeyspaceTableIDCommand with arg "16777215", and asserts the captured
request's "key" equals codec.EncodeBytes(nil, []byte{'x',0xFF,0xFF,0xFF}) and
"end_key" equals codec.EncodeBytes(nil, []byte{'y'}) (use mustQueryUnescape to
decode query values) and restore dialClient in a defer; this ensures the end_key
uses the non-wrapping next prefix for the 3-byte keyspace field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c278144-f282-43de-a635-fa37d1e18311
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/pdctl/command/region_test.go
Member
|
duplicated with #9850 |
Contributor
Author
|
/retest |
Contributor
Author
|
/closed #9850 |
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 #10516, closed #10531
This PR ports the
pd-ctlregion lookup enhancement tracked intikv/pd#10516onto the currentmasterbranch so operators can query regions by keyspace ID and optional table ID.Source change:
bufferflies/pd@c93399a176f8ffc13580fe7ef35c43aa3830902e.What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit