tools: pdctl support query region direct#10273
Conversation
Signed-off-by: tongjian <1045931706@qq.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
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 a per-request Changes
Sequence DiagramsequenceDiagram
participant Client as pd-ctl Client
participant Follower as PD Follower
participant Leader as PD Leader
alt pd-ctl without --direct
Client->>Follower: Region request (no PDAllowFollowerHandleHeader)
Follower->>Leader: Forward request to leader
Note over Leader: Failpoint may set regionInfo = nil → 404
Leader-->>Follower: 404 / region info
Follower-->>Client: 404 / region info
else pd-ctl with --direct
Client->>Client: buildDirectHeader() sets PDAllowFollowerHandleHeader=true
Client->>Follower: Region request (header present)
Note over Follower: Handle locally when header=true
Follower-->>Client: region info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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/L
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10273 +/- ##
==========================================
+ Coverage 78.80% 78.93% +0.12%
==========================================
Files 523 530 +7
Lines 70529 71561 +1032
==========================================
+ Hits 55580 56484 +904
- Misses 10955 11057 +102
- Partials 3994 4020 +26
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/pd-ctl/tests/region/region_test.go (1)
561-564: Consider strengthening the follower assertion.The assertion accepts either region data (
"id":100) OR"TiKV cluster not bootstrapped". This fallback suggests potential test flakiness - if followers are properly handling the request, they should consistently return region data since the cluster was bootstrapped and the region heartbeat was processed.If the "not bootstrapped" case is a known race condition, consider adding a brief wait or retry logic to ensure cluster state propagation before querying followers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/region/region_test.go` around lines 561 - 564, The current test reads output from tests.ExecuteCommand(cmd, "-u", serverAddr, "region", "100", "--direct") and accepts either "\"id\":100" or "TiKV cluster not bootstrapped", which masks flakiness; modify the test to retry the ExecuteCommand call (or add a short wait loop) until the response contains "\"id\":100" or a timeout is reached, so followers are given time to observe the bootstrapped/heartbeat state before asserting, and remove the permissive OR branch once retries/timeouts are exhausted; reference tests.ExecuteCommand and the region query parameters ("region", "100", "--direct") when implementing the wait/retry.tools/pd-ctl/tests/global_test.go (1)
130-142: Confusing test behavior:--direct=falsestill adds headers.The comment states the direct flag "does not control whether to add X-Forwarded-For header," but this seems counterintuitive. The test expects
--direct=falseto still add headers becausebuildDirectHeaderchecksChanged("direct")rather than the flag's actual value.If this is intentional behavior, consider clarifying the flag's purpose in its help text. If
--direct=falseshould disable header addition, the condition inbuildDirectHeadershould check the flag's boolean value, not just whether it was changed.♻️ If the intent is that --direct=false should NOT add headers
In
global.go:- if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") { + if direct, _ := cmd.Flags().GetBool("direct"); direct {And update this test to expect empty headers for
--direct=false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/global_test.go` around lines 130 - 142, The test observes that --direct=false still causes X-Forwarded-For to be added because buildDirectHeader only checks whether the flag was changed (Changed("direct")) rather than its boolean value; change buildDirectHeader to read the actual boolean value of the "direct" flag (e.g., via the command's FlagSet/GetBool or the flags API used in your CLI) and only add the X-Forwarded-For header when that boolean is true, and update the test (the ExecuteCommand call with "--direct=false") to expect no forwarded header when the flag is false.
🤖 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/global.go`:
- Around line 193-205: The buildDirectHeader function uses
cmd.Flags().Lookup("direct") which only sees local flags; change it to use
cmd.Flag("direct") so inherited persistent flags are detected (check the
returned *pflag.Flag for nil and use its .Changed property). Update the
conditional that currently references flag := cmd.Flags().Lookup("direct") to
obtain the flag via cmd.Flag("direct") and keep the rest of the logic (iterating
getEndpoints(cmd), adding apiutil.XForwardedForHeader, and setting
apiutil.PDAllowFollowerHandleHeader) unchanged.
---
Nitpick comments:
In `@tools/pd-ctl/tests/global_test.go`:
- Around line 130-142: The test observes that --direct=false still causes
X-Forwarded-For to be added because buildDirectHeader only checks whether the
flag was changed (Changed("direct")) rather than its boolean value; change
buildDirectHeader to read the actual boolean value of the "direct" flag (e.g.,
via the command's FlagSet/GetBool or the flags API used in your CLI) and only
add the X-Forwarded-For header when that boolean is true, and update the test
(the ExecuteCommand call with "--direct=false") to expect no forwarded header
when the flag is false.
In `@tools/pd-ctl/tests/region/region_test.go`:
- Around line 561-564: The current test reads output from
tests.ExecuteCommand(cmd, "-u", serverAddr, "region", "100", "--direct") and
accepts either "\"id\":100" or "TiKV cluster not bootstrapped", which masks
flakiness; modify the test to retry the ExecuteCommand call (or add a short wait
loop) until the response contains "\"id\":100" or a timeout is reached, so
followers are given time to observe the bootstrapped/heartbeat state before
asserting, and remove the permissive OR branch once retries/timeouts are
exhausted; reference tests.ExecuteCommand and the region query parameters
("region", "100", "--direct") when implementing the wait/retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13618ab6-2a5d-4acc-bc09-ff35235fc6b2
📒 Files selected for processing (5)
server/api/region.gotools/pd-ctl/pdctl/command/global.gotools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/tests/global_test.gotools/pd-ctl/tests/region/region_test.go
| if header == nil { | ||
| header = http.Header{} | ||
| } | ||
| if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") { |
There was a problem hiding this comment.
cmd.Flags().Changed("direct") is also true for --direct=false, so this will still enable the direct path when the flag is explicitly set to false.
There was a problem hiding this comment.
If the user doesn't want to direct mod, he can remove this flag.
| for _, endpoint := range getEndpoints(cmd) { | ||
| header.Add(apiutil.XForwardedForHeader, endpoint) | ||
| } |
There was a problem hiding this comment.
We should remove it, PD-Allow-follower-handle is enough.
We should not set X-Forwarded-For to PD endpoints. PD treats X-Forwarded-For as the caller/source IP, so adding PD addresses here changes the request metadata unexpectedly.
For example, with -u pd1:2379,pd2:2379,pd3:2379, the request logic may still try all endpoints, but the server-side parsing of X-Forwarded-For will effectively see the first value and record pd1:2379 as the source, which is incorrect.
There was a problem hiding this comment.
good catch, done
Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/pd-ctl/pdctl/command/global.go (1)
193-200:⚠️ Potential issue | 🟠 MajorGate follower handling on the
--directvalue, not just flag existence.Line 198 only checks whether the flag exists. Since
region_command.goregistersdirectas a bool flag defaulting tofalse, every region subcommand now sendsPD-Allow-Follower-Handle: trueeven when the user did not opt into direct mode.🐛 Proposed fix
func buildDirectHeader(cmd *cobra.Command, customHeader http.Header) http.Header { header := customHeader.Clone() if header == nil { header = http.Header{} } - if flag := cmd.Flags().Lookup("direct"); flag != nil { + if direct, err := cmd.Flags().GetBool("direct"); err == nil && direct { header.Set(apiutil.PDAllowFollowerHandleHeader, "true") } return header }#!/bin/bash set -euo pipefail printf 'direct flag definition:\n' rg -n -C2 'PersistentFlags\(\)\.Bool\("direct"' tools/pd-ctl/pdctl/command printf '\ncurrent header gate:\n' rg -n -C3 'func buildDirectHeader|Lookup\("direct"\)|GetBool\("direct"\)|PDAllowFollowerHandleHeader' tools/pd-ctl/pdctl/command/global.goExpected result: the first match shows
directis a bool flag with defaultfalse; the second shows the helper currently enables follower handling based on flag presence alone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/global.go` around lines 193 - 200, The buildDirectHeader helper currently enables PD-Allow-Follower-Handle whenever the "direct" flag exists; change it to gate on the flag's boolean value instead. In buildDirectHeader, replace the existence check (cmd.Flags().Lookup("direct") != nil) with a retrieval of the boolean value (e.g., use cmd.Flags().GetBool("direct") or inspect the Lookup("direct").Value) and only call header.Set(apiutil.PDAllowFollowerHandleHeader, "true") when that boolean is true; keep using customHeader.Clone() and the same header variable names.
🤖 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/global.go`:
- Around line 193-200: The buildDirectHeader helper currently enables
PD-Allow-Follower-Handle whenever the "direct" flag exists; change it to gate on
the flag's boolean value instead. In buildDirectHeader, replace the existence
check (cmd.Flags().Lookup("direct") != nil) with a retrieval of the boolean
value (e.g., use cmd.Flags().GetBool("direct") or inspect the
Lookup("direct").Value) and only call
header.Set(apiutil.PDAllowFollowerHandleHeader, "true") when that boolean is
true; keep using customHeader.Clone() and the same header variable names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a4e9b1-b643-4a69-a38d-0b76c213abe3
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/global.gotools/pd-ctl/pdctl/command/region_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pd-ctl/pdctl/command/region_command.go
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/tests/global_test.go`:
- Around line 93-100: The test currently overwrites allowFollowerByRawQuery[""]
on each /pd/api/v1/regions request so earlier observations are lost; update the
handler in the mux.HandleFunc (the closure that captures allowFollowerByRawQuery
and reads apiutil.PDAllowFollowerHandleHeader) to record each observed request
instead of replacing the slice — e.g. append the new header values onto the
existing slice for the given r.URL.RawQuery
(allowFollowerByRawQuery[r.URL.RawQuery] =
append(allowFollowerByRawQuery[r.URL.RawQuery],
r.Header.Values(apiutil.PDAllowFollowerHandleHeader)...)) or, alternatively,
ensure the test clears the map entry before each ExecuteCommand so each
assertion checks a fresh observation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92e10253-624a-4b47-86eb-af697f223db0
📒 Files selected for processing (1)
tools/pd-ctl/tests/global_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/pd-ctl/tests/global_test.go (1)
127-142: Swap assertion arguments to follow testify convention.In
require.Equal(t, expected, actual, ...), the first argument should be the expected value and the second the actual value. This ensures clearer failure messages (e.g., "expected 0 but got 1" vs "expected 1 but got 0").♻️ Suggested fix
- re.Equal(count, 0) + re.Equal(0, count) // The direct flag only controls endpoint selection. PD-Allow-follower-handle is always added // for region requests, regardless of the direct flag value. _, err = ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct") re.NoError(err) - re.Equal(count, 1) + re.Equal(1, count) // Ignore direct flag value. _, err = ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct=false") re.NoError(err) - re.Equal(count, 2) + re.Equal(2, count) _, err = ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct=true") re.NoError(err) - re.Equal(count, 3) + re.Equal(3, count)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/global_test.go` around lines 127 - 142, The assertions in global_test.go use testify's Equal with arguments reversed; swap the expected and actual for the re.Equal calls that check count after ExecuteCommand invocations (the calls around ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct"), ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct=false"), and ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct=true") ): change re.Equal(count, 0) to re.Equal(0, count), re.Equal(count, 1) to re.Equal(1, count), re.Equal(count, 2) to re.Equal(2, count), and re.Equal(count, 3) to re.Equal(3, count) so expected values come first and actual (count) second.
🤖 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/tests/global_test.go`:
- Around line 129-130: The comment is incorrect: update the comment to reflect
that buildDirectHeader adds the "PD-Allow-follower-handle" header only when the
--direct flag was explicitly provided (flag.Changed is true), regardless of the
flag's boolean value; it is not "always added" for region requests. Mention
buildDirectHeader and the --direct flag (and flag.Changed behavior) so readers
understand the header presence is tied to the flag being passed, not its
true/false value.
---
Nitpick comments:
In `@tools/pd-ctl/tests/global_test.go`:
- Around line 127-142: The assertions in global_test.go use testify's Equal with
arguments reversed; swap the expected and actual for the re.Equal calls that
check count after ExecuteCommand invocations (the calls around
ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct"), ExecuteCommand(cmd,
"-u", pdAddr, "region", "--direct=false"), and ExecuteCommand(cmd, "-u", pdAddr,
"region", "--direct=true") ): change re.Equal(count, 0) to re.Equal(0, count),
re.Equal(count, 1) to re.Equal(1, count), re.Equal(count, 2) to re.Equal(2,
count), and re.Equal(count, 3) to re.Equal(3, count) so expected values come
first and actual (count) second.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 687b950e-d8f3-4f24-8c6f-406d903bd983
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/global.gotools/pd-ctl/tests/global_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/pd-ctl/pdctl/command/global.go
bufferflies
force-pushed
the
pdctl/region-direct
branch
from
43dcbd7 to
c256a56
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/pd-ctl/tests/global_test.go (1)
129-130:⚠️ Potential issue | 🟡 MinorFix contradictory comment about header behavior.
The comment says the header is “always added,” but Line 127 explicitly verifies it is not added when
--directis omitted.📝 Proposed comment fix
- // The direct flag only controls endpoint selection. PD-Allow-follower-handle is always added - // for region requests, regardless of the direct flag value. + // PD-Allow-follower-handle is added only when --direct is explicitly passed, + // regardless of whether the passed value is true or false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/global_test.go` around lines 129 - 130, The comment is contradictory: it claims the "PD-Allow-follower-handle" header is "always added" but the test verifies it is not added when the "--direct" flag is omitted; update the comment near the test that references the direct flag to state the correct behavior — that the "PD-Allow-follower-handle" header is only added for region requests when the "--direct" flag is present (or when direct mode is enabled), and not added when "--direct" is omitted, so the comment matches the assertion in the test.
🧹 Nitpick comments (2)
tools/pd-ctl/tests/global_test.go (1)
100-102: Assert header value ("true"), not only presence.This test currently passes even if the header is present with an unexpected value.
🧪 Suggested tightening
- if vals := r.Header.Values(apiutil.PDAllowFollowerHandleHeader); len(vals) > 0 { + if r.Header.Get(apiutil.PDAllowFollowerHandleHeader) == "true" { count++ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/global_test.go` around lines 100 - 102, The test currently only checks presence of the PDAllowFollowerHandleHeader by counting any non-empty r.Header.Values(apiutil.PDAllowFollowerHandleHeader); update the assertion to validate the header value is exactly "true" instead of merely present: locate the check using r.Header.Values(apiutil.PDAllowFollowerHandleHeader) in the test and replace or augment it with an explicit equality check (e.g., assert/require that r.Header.Get(apiutil.PDAllowFollowerHandleHeader) == "true" or that the first element of vals equals "true") so the test fails on unexpected header values.tools/pd-ctl/pdctl/command/global.go (1)
198-200: Remove stale commented-out logic inbuildDirectHeader.Leaving old conditional code commented here adds noise and can confuse future readers.
♻️ Proposed cleanup
- // if flag := cmd.Flags().Lookup("direct"); flag != nil { - // header.Set(apiutil.PDAllowFollowerHandleHeader, "true") - // }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/global.go` around lines 198 - 200, Remove the stale commented-out conditional inside buildDirectHeader: delete the commented block that checks cmd.Flags().Lookup("direct") and sets apiutil.PDAllowFollowerHandleHeader, leaving only active code. This cleans up noise in the buildDirectHeader function and avoids confusing future readers.
🤖 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/tests/global_test.go`:
- Around line 129-130: The comment is contradictory: it claims the
"PD-Allow-follower-handle" header is "always added" but the test verifies it is
not added when the "--direct" flag is omitted; update the comment near the test
that references the direct flag to state the correct behavior — that the
"PD-Allow-follower-handle" header is only added for region requests when the
"--direct" flag is present (or when direct mode is enabled), and not added when
"--direct" is omitted, so the comment matches the assertion in the test.
---
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/global.go`:
- Around line 198-200: Remove the stale commented-out conditional inside
buildDirectHeader: delete the commented block that checks
cmd.Flags().Lookup("direct") and sets apiutil.PDAllowFollowerHandleHeader,
leaving only active code. This cleans up noise in the buildDirectHeader function
and avoids confusing future readers.
In `@tools/pd-ctl/tests/global_test.go`:
- Around line 100-102: The test currently only checks presence of the
PDAllowFollowerHandleHeader by counting any non-empty
r.Header.Values(apiutil.PDAllowFollowerHandleHeader); update the assertion to
validate the header value is exactly "true" instead of merely present: locate
the check using r.Header.Values(apiutil.PDAllowFollowerHandleHeader) in the test
and replace or augment it with an explicit equality check (e.g., assert/require
that r.Header.Get(apiutil.PDAllowFollowerHandleHeader) == "true" or that the
first element of vals equals "true") so the test fails on unexpected header
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c10e0868-a14e-46dc-a04c-91f400e9fa44
📒 Files selected for processing (2)
tools/pd-ctl/pdctl/command/global.gotools/pd-ctl/tests/global_test.go
Signed-off-by: tongjian <1045931706@qq.com>
bufferflies
force-pushed
the
pdctl/region-direct
branch
from
c256a56 to
fd699e4
Compare
|
/retest |
| header = http.Header{} | ||
| } | ||
|
|
||
| if flag := cmd.Flag("direct"); flag != nil && flag.Changed { |
There was a problem hiding this comment.
flag.Changed? What will happen when “--direct=false”?
| // Ignore direct flag value. | ||
| _, err = ExecuteCommand(cmd, "-u", pdAddr, "region", "--direct=false") | ||
| re.NoError(err) | ||
| re.Equal(2, count) |
There was a problem hiding this comment.
Should we use direct as long as the flag exists, regardless of whether it's true or false? That seems a bit strange.
Signed-off-by: tongjian <1045931706@qq.com>
|
/retest |
ti-chi-bot
Bot
added
needs-1-more-lgtm
Signed-off-by: tongjian <1045931706@qq.com>
bufferflies
force-pushed
the
pdctl/region-direct
branch
from
ed81edb to
dc69e4e
Compare
| return resp, err | ||
| } | ||
|
|
||
| func buildDirectHeader(cmd *cobra.Command, customHeader http.Header) http.Header { |
There was a problem hiding this comment.
please update other naming together
Signed-off-by: tongjian <1045931706@qq.com>
|
/retest |
|
/ping @okJiang |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, okJiang 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
@bufferflies: 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. |
3 participants
What problem does this PR solve?
Issue Number: Close #10327
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
New Features
Tests