pd: replace client with NewClientWithAPIContext#58561
pd: replace client with NewClientWithAPIContext#58561lhy1024 wants to merge 1 commit intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
Hi @lhy1024. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #58561 +/- ##
================================================
+ Coverage 77.5894% 79.0000% +1.4106%
================================================
Files 1982 1992 +10
Lines 548964 549144 +180
================================================
+ Hits 425938 433824 +7886
+ Misses 122221 113873 -8348
- Partials 805 1447 +642
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[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 |
📝 WalkthroughWalkthroughAdded keyspace-aware PD integration: New keyspace parameter flows into manager and PD controller constructors, PD clients switch to API-context variants, a keyspace helper was added, and several call sites and tests were updated to pass/accept the keyspace name. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Caller
participant Mgr as Manager (task.NewMgr)
participant Conn as conn.NewMgr / pdutil
participant PD as PD Client
participant TiKV as TiKV / Storage
CLI->>Mgr: call NewMgr(ctx, glue, keyspaceName, pdAddrs, ...)
Mgr->>Conn: forward keyspaceName + pdAddrs
Conn->>PD: pd.NewClientWithAPIContext(ctx, BuildAPIContext(keyspaceName), ...)
PD->>TiKV: keyspace-scoped PD API resolves stores/TSO
PD-->>Conn: PD client ready
Conn-->>Mgr: connection manager returned
Mgr-->>CLI: manager usable for backup/restore/stream ops
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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: 3
♻️ Duplicate comments (2)
br/pkg/task/restore_raw.go (1)
77-77:⚠️ Potential issue | 🟡 MinorSame flag-registration gap —
--keyspace-nameisn't wired into raw restore CLI either.
DefineRawRestoreFlags+RestoreRawConfig.ParseFromFlagsdon't readflagKeyspaceName, socfg.KeyspaceNamepassed here is always empty. Call-site itself matches the newNewMgrsignature correctly; flag-wiring is the only loose end. See verification comment onbackup_txn.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/task/restore_raw.go` at line 77, The CLI flag `--keyspace-name` (flagKeyspaceName) isn't being wired into the raw restore config, so cfg.KeyspaceName is always empty when calling NewMgr; fix this by adding the flag registration in DefineRawRestoreFlags and reading the value into RestoreRawConfig in RestoreRawConfig.ParseFromFlags (set the RestoreRawConfig.KeyspaceName field from flagKeyspaceName), ensuring the same flag name/value handling as used by the backup path so that cfg.KeyspaceName is populated before NewMgr is called.br/pkg/task/backup_raw.go (1)
142-142:⚠️ Potential issue | 🟡 MinorSame flag-registration gap as
backup_txn.go.
DefineRawBackupFlags/RawKvConfig.ParseFromFlagsdon't register or read--keyspace-name, socfg.KeyspaceNamewill always be empty for raw backup as well. See the verification comment onbackup_txn.goline 121.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/task/backup_raw.go` at line 142, The raw-backup flow is missing registration/parsing of the --keyspace-name flag, so cfg.KeyspaceName is always empty; update DefineRawBackupFlags and/or RawKvConfig.ParseFromFlags to register and read the keyspace-name flag (same fix applied in backup_txn.go) so that cfg.KeyspaceName is populated before NewMgr is called in backup_raw.go; ensure the flag name matches usage, add parsing to assign into RawKvConfig.KeyspaceName (or cfg.KeyspaceName) and include a unit/verification similar to the backup_txn.go fix.
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
94-103: Optional: simplifyBuildAPIContextand enrich the doc comment.The named return with
if/elsebranches can be a single-line return. Additionally, the doc comment restates the name — briefly noting the semantics (empty keyspace → V1, otherwise V2) would help readers.♻️ Proposed refactor
-// BuildAPIContext is used to build APIContext. -func BuildAPIContext(keyspaceName string) (apiContext pd.APIContext) { - if len(keyspaceName) == 0 { - apiContext = pd.NewAPIContextV1() - } else { - apiContext = pd.NewAPIContextV2(keyspaceName) - } - return -} +// BuildAPIContext returns a PD APIContext for the given keyspace name. +// An empty keyspaceName yields a V1 context; otherwise a V2 context scoped +// to the keyspace is returned. +func BuildAPIContext(keyspaceName string) pd.APIContext { + if len(keyspaceName) == 0 { + return pd.NewAPIContextV1() + } + return pd.NewAPIContextV2(keyspaceName) +}As per coding guidelines: "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 94 - 103, Update the BuildAPIContext function and its doc comment: change the comment to briefly state the semantics ("returns NewAPIContextV1 when keyspaceName is empty, otherwise NewAPIContextV2") and simplify the implementation to a single-line return that calls pd.NewAPIContextV1() when keyspaceName == "" else pd.NewAPIContextV2(keyspaceName); keep the function name BuildAPIContext and the pd.NewAPIContextV1/pd.NewAPIContextV2 calls as the referenced symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@br/pkg/conn/conn.go`:
- Line 155: The TiKV storage URL is currently built with
config.GetGlobalKeyspaceName() while the PD controller (constructed in NewMgr
with the keyspaceName parameter) uses the passed-in keyspaceName; update the
TiKV storage path construction in NewMgr to use the keyspaceName parameter
instead of config.GetGlobalKeyspaceName() so both PD controller and TiKV codec
operate on the same keyspace (affects code paths used by BackupRaw/RestoreRaw
that don't call config.UpdateGlobal()).
In `@br/pkg/task/backup_txn.go`:
- Line 121: The txn backup CLI never gets a keyspace value because
DefineTxnBackupFlags doesn't register --keyspace-name and
TxnKvConfig.ParseFromFlags doesn't populate cfg.KeyspaceName, yet
cfg.KeyspaceName is passed into NewMgr; add support by registering the
"--keyspace-name" flag in DefineTxnBackupFlags and reading it into
TxnKvConfig.ParseFromFlags so cfg.KeyspaceName is populated (match the same flag
name/behavior used in backup.go), then pass that populated cfg.KeyspaceName to
NewMgr; alternatively, if txn backups truly don't use keyspace scoping, remove
the cfg.KeyspaceName parameter from the NewMgr call and corresponding function
signatures and document the decision.
In `@pkg/store/store.go`:
- Around line 147-153: The current IsNotTSOLeaderError function is too broad
(strings.Contains(err.Error(), "not leader")) and matches non-TSO components;
update IsNotTSOLeaderError to only detect TSO/PD-specific "not leader" errors by
either (a) matching a PD/Tso-specific signature (e.g., look for "tso" or "pd"
context in the message such as "tso: not leader" or "pd: not leader") or
preferably (b) performing a typed check against the PD/TSO client error (use
errors.Is or the PD client error type/constant) inside IsNotTSOLeaderError; keep
the symbol name IsNotTSOLeaderError and ensure isNewStoreRetryableError uses
this narrowed check, or if you intend to keep the broad behavior, rename and
re-document the function to reflect it matches any "not leader" error.
---
Duplicate comments:
In `@br/pkg/task/backup_raw.go`:
- Line 142: The raw-backup flow is missing registration/parsing of the
--keyspace-name flag, so cfg.KeyspaceName is always empty; update
DefineRawBackupFlags and/or RawKvConfig.ParseFromFlags to register and read the
keyspace-name flag (same fix applied in backup_txn.go) so that cfg.KeyspaceName
is populated before NewMgr is called in backup_raw.go; ensure the flag name
matches usage, add parsing to assign into RawKvConfig.KeyspaceName (or
cfg.KeyspaceName) and include a unit/verification similar to the backup_txn.go
fix.
In `@br/pkg/task/restore_raw.go`:
- Line 77: The CLI flag `--keyspace-name` (flagKeyspaceName) isn't being wired
into the raw restore config, so cfg.KeyspaceName is always empty when calling
NewMgr; fix this by adding the flag registration in DefineRawRestoreFlags and
reading the value into RestoreRawConfig in RestoreRawConfig.ParseFromFlags (set
the RestoreRawConfig.KeyspaceName field from flagKeyspaceName), ensuring the
same flag name/value handling as used by the backup path so that
cfg.KeyspaceName is populated before NewMgr is called.
---
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 94-103: Update the BuildAPIContext function and its doc comment:
change the comment to briefly state the semantics ("returns NewAPIContextV1 when
keyspaceName is empty, otherwise NewAPIContextV2") and simplify the
implementation to a single-line return that calls pd.NewAPIContextV1() when
keyspaceName == "" else pd.NewAPIContextV2(keyspaceName); keep the function name
BuildAPIContext and the pd.NewAPIContextV1/pd.NewAPIContextV2 calls as the
referenced symbols.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45cc2209-f795-47f2-962d-c7d477e3f320
📒 Files selected for processing (28)
br/cmd/br/debug.gobr/pkg/conn/conn.gobr/pkg/pdutil/BUILD.bazelbr/pkg/pdutil/pd.gobr/pkg/task/backup.gobr/pkg/task/backup_ebs.gobr/pkg/task/backup_raw.gobr/pkg/task/backup_txn.gobr/pkg/task/common.gobr/pkg/task/operator/checksum_table.gobr/pkg/task/operator/crr_checkpoint.gobr/pkg/task/operator/prepare_snap.gobr/pkg/task/restore.gobr/pkg/task/restore_data.gobr/pkg/task/restore_ebs_meta.gobr/pkg/task/restore_raw.gobr/pkg/task/restore_txn.gobr/pkg/task/stream.golightning/pkg/importer/import.gopkg/dxf/importinto/planner_test.gopkg/executor/importer/import.gopkg/executor/importer/table_import.gopkg/executor/importer/table_import_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace.gopkg/lightning/backend/local/BUILD.bazelpkg/lightning/backend/local/local.gopkg/store/store.go
| // IsNotTSOLeaderError returns true if the error is caused by not a TSO leader. | ||
| func IsNotTSOLeaderError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| return strings.Contains(err.Error(), "not leader") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, confirm the file exists and read the relevant section
if [ -f "pkg/store/store.go" ]; then
echo "=== Lines 140-160 of pkg/store/store.go ==="
sed -n '140,160p' pkg/store/store.go
else
echo "File not found at pkg/store/store.go"
fiRepository: pingcap/tidb
Length of output: 755
🏁 Script executed:
# Search for IsNotTSOLeaderError usage in the codebase
echo "=== Usage of IsNotTSOLeaderError ==="
rg "IsNotTSOLeaderError" --type go -A 3 -B 1Repository: pingcap/tidb
Length of output: 678
🏁 Script executed:
# Search for isNewStoreRetryableError to understand the call chain
echo "=== isNewStoreRetryableError function and usage ==="
rg "isNewStoreRetryableError" --type go -B 2 -A 5Repository: pingcap/tidb
Length of output: 979
🏁 Script executed:
# Check for "not leader" errors in the mentioned files
echo "=== Checking for 'not leader' in autoid_service ==="
if [ -f "pkg/autoid_service/autoid.go" ]; then
rg "not leader" pkg/autoid_service/autoid.go -B 2 -A 2
else
echo "File not found"
fi
echo ""
echo "=== Checking for 'not leader' in ingestor ==="
if [ -f "pkg/ingestor/errdef/errors.go" ]; then
rg "not leader" pkg/ingestor/errdef/errors.go -B 2 -A 2
else
echo "File not found"
fi
echo ""
echo "=== Checking for 'not leader' in BR ==="
if [ -f "br/pkg/errors/errors.go" ]; then
rg "not leader" br/pkg/errors/errors.go -B 2 -A 2
else
echo "File not found"
fiRepository: pingcap/tidb
Length of output: 2216
🏁 Script executed:
# Search for ErrKVNotLeader in the codebase to understand these errors better
echo "=== ErrKVNotLeader references ==="
rg "ErrKVNotLeader" --type go -B 1 -A 1Repository: pingcap/tidb
Length of output: 3677
IsNotTSOLeaderError matches unrelated component errors due to broad substring matching.
The function is documented as TSO-leader-specific ("returns true if the error is caused by not a TSO leader"), but strings.Contains(err.Error(), "not leader") will also match "not leader" errors from unrelated components: the AutoID service (pkg/autoid_service/autoid.go), ingestor (pkg/ingestor/errdef/errors.go), and BR (br/pkg/errors/errors.go). Since this check feeds isNewStoreRetryableError, any such error during store initialization will incorrectly trigger store-creation retries.
Consider narrowing the match to a TSO-specific signature (e.g., include a PD-related context or match on a typed PD error) rather than matching any "not leader" substring, or update the function name and documentation to reflect the broader matching behavior if that is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/store/store.go` around lines 147 - 153, The current IsNotTSOLeaderError
function is too broad (strings.Contains(err.Error(), "not leader")) and matches
non-TSO components; update IsNotTSOLeaderError to only detect TSO/PD-specific
"not leader" errors by either (a) matching a PD/Tso-specific signature (e.g.,
look for "tso" or "pd" context in the message such as "tso: not leader" or "pd:
not leader") or preferably (b) performing a typed check against the PD/TSO
client error (use errors.Is or the PD client error type/constant) inside
IsNotTSOLeaderError; keep the symbol name IsNotTSOLeaderError and ensure
isNewStoreRetryableError uses this narrowed check, or if you intend to keep the
broad behavior, rename and re-document the function to reflect it matches any
"not leader" error.
Squashed from tidbcloud/tidb-cse#1899. Signed-off-by: lhy1024 <admin@liudos.us>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
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 `@br/pkg/task/restore_txn.go`:
- Line 33: runRestoreTxnCommand is not picking up the --keyspace-name flag
because task.Config.ParseFromFlags() doesn't read FlagKeyspaceName; ensure
cfg.KeyspaceName is populated before calling NewMgr by reading the persistent
flag added by DefineRestoreFlags. Fix by retrieving the value of
FlagKeyspaceName from the command flags (e.g.
cmd.Flags().GetString(FlagKeyspaceName) or cmd.PersistentFlags().GetString) or
by invoking the same parsing used by RestoreConfig.ParseFromFlags(), then assign
it into cfg.KeyspaceName so RunRestoreTxn and NewMgr receive the correct
keyspace value.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 030699bc-56dc-410e-8c3a-36c7222f78c6
📒 Files selected for processing (28)
br/cmd/br/debug.gobr/pkg/conn/conn.gobr/pkg/pdutil/BUILD.bazelbr/pkg/pdutil/pd.gobr/pkg/task/backup.gobr/pkg/task/backup_ebs.gobr/pkg/task/backup_raw.gobr/pkg/task/backup_txn.gobr/pkg/task/common.gobr/pkg/task/operator/checksum_table.gobr/pkg/task/operator/crr_checkpoint.gobr/pkg/task/operator/prepare_snap.gobr/pkg/task/restore.gobr/pkg/task/restore_data.gobr/pkg/task/restore_ebs_meta.gobr/pkg/task/restore_raw.gobr/pkg/task/restore_txn.gobr/pkg/task/stream.golightning/pkg/importer/import.gopkg/dxf/importinto/planner_test.gopkg/executor/importer/import.gopkg/executor/importer/table_import.gopkg/executor/importer/table_import_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace.gopkg/lightning/backend/local/BUILD.bazelpkg/lightning/backend/local/local.gopkg/store/store.go
✅ Files skipped from review due to trivial changes (6)
- br/pkg/pdutil/BUILD.bazel
- br/pkg/task/restore_raw.go
- br/pkg/task/operator/checksum_table.go
- pkg/keyspace/BUILD.bazel
- br/pkg/task/restore_data.go
- pkg/lightning/backend/local/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (16)
- br/pkg/task/operator/crr_checkpoint.go
- br/pkg/task/backup_raw.go
- br/pkg/task/operator/prepare_snap.go
- br/pkg/task/backup_ebs.go
- pkg/executor/importer/import.go
- br/pkg/task/stream.go
- br/cmd/br/debug.go
- br/pkg/task/restore_ebs_meta.go
- pkg/executor/importer/table_import_test.go
- pkg/keyspace/keyspace.go
- pkg/lightning/backend/local/local.go
- pkg/store/store.go
- br/pkg/task/restore.go
- br/pkg/pdutil/pd.go
- br/pkg/conn/conn.go
- br/pkg/task/common.go
|
|
||
| // Restore raw does not need domain. | ||
| mgr, err := NewMgr(ctx, g, cfg.PD, cfg.TLS, GetKeepalive(cfg), cfg.CheckRequirements, false, conn.NormalVersionChecker) | ||
| mgr, err := NewMgr(ctx, g, cfg.KeyspaceName, cfg.PD, cfg.TLS, GetKeepalive(cfg), cfg.CheckRequirements, false, conn.NormalVersionChecker) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate txn restore flag definitions and confirm whether flagKeyspaceName is registered.
rg -nP '\bDefineRestoreTxn|DefineTxnRestoreFlags|DefineRestoreFlags\b' -C3
# Find the cobra command wiring for RunRestoreTxn
rg -nP '\bRunRestoreTxn\b' -C5
# Check which flag sets register flagKeyspaceName
rg -nP '\bflagKeyspaceName\b' -C2Repository: pingcap/tidb
Length of output: 4953
🏁 Script executed:
# Get the complete DefineRestoreFlags function
sed -n '366,450p' br/pkg/task/restore.go
# Check how RunRestoreTxn and NewMgr use the keyspace name
sed -n '1,80p' br/pkg/task/restore_txn.go
# Check Config struct to understand the type being used
rg -nP 'type Config struct' br/pkg/task/ -A 30 | head -100Repository: pingcap/tidb
Length of output: 11195
🏁 Script executed:
# Find FlagKeyspaceName definition
rg -nP '\bFlagKeyspaceName\s*=' br/pkg/task/
# Check Config.ParseFromFlags to see if it reads FlagKeyspaceName
sed -n '600,750p' br/pkg/task/restore.go | grep -A 50 'ParseFromFlags'
# Check the complete backup.go and restore.go for flag constant definitions
rg -nP '^const \(' br/pkg/task/ -A 50 | grep -E 'keyspace|Keyspace'
# Check how TxnKvConfig differs from RestoreConfig
rg -nP 'type TxnKvConfig struct' br/pkg/task/ -A 20Repository: pingcap/tidb
Length of output: 2230
🏁 Script executed:
# Get the complete Config.ParseFromFlags method from common.go to see if it reads FlagKeyspaceName
rg -nP 'func \(cfg \*Config\) ParseFromFlags' br/pkg/task/common.go -A 100 | head -150
# Also check where the restore command (including txn restore) parses the config
rg -nP 'ParseFromFlags|newTxnRestoreCommand' br/cmd/br/restore.go -B 3 -A 10Repository: pingcap/tidb
Length of output: 6145
🏁 Script executed:
# Check if TxnKvConfig has its own ParseFromFlags or if it inherits from Config
rg -nP 'func \(cfg \*TxnKvConfig\) ParseFromFlags' br/pkg/task/
# Check where RestoreConfig.ParseFromFlags reads FlagKeyspaceName
rg -nP 'ParseFromFlags.*RestoreConfig' br/pkg/task/restore.go -A 200 | grep -i keyspace
# Check the complete RestoreConfig.ParseFromFlags implementation
sed -n '900,1050p' br/pkg/task/restore.go
# Check if newTxnRestoreCommand actually calls DefineRestoreFlags somewhere
grep -n 'DefineRestoreFlags\|DefineRawRestoreFlags' br/cmd/br/restore.goRepository: pingcap/tidb
Length of output: 5265
🏁 Script executed:
# Check DefineRawRestoreFlags to see if it registers FlagKeyspaceName
rg -nP 'func DefineRawRestoreFlags' br/pkg/task/ -A 30
# Check if Config.ParseFromFlags reads FlagKeyspaceName (checking more lines)
sed -n '610,750p' br/pkg/task/common.go | grep -i keyspace
# Check what flags are available when runRestoreTxnCommand is called
rg -nP 'runRestoreTxnCommand|newTxnRestoreCommand' br/cmd/br/restore.go -B 5 -A 15Repository: pingcap/tidb
Length of output: 4203
🏁 Script executed:
# Check if RestoreConfig.ParseFromFlags reads FlagKeyspaceName
rg -nP 'func \(cfg \*RestoreConfig\) ParseFromFlags' br/pkg/task/ -A 100 | head -150
# Check DefineRestoreCommonFlags to understand what flags it registers
rg -nP 'func DefineRestoreCommonFlags' br/pkg/task/ -A 30Repository: pingcap/tidb
Length of output: 9205
--keyspace-name flag is registered but not parsed for txn restore.
FlagKeyspaceName is registered via DefineRestoreFlags() on the parent restore command's PersistentFlags, so it's available to the txn restore subcommand. However, runRestoreTxnCommand() uses plain task.Config and calls Config.ParseFromFlags(), which does not attempt to read FlagKeyspaceName. Only RestoreConfig.ParseFromFlags() reads this flag (line 510 of restore.go). As a result, even though the flag is registered and available, passing --keyspace-name for txn restore will be silently ignored, leaving cfg.KeyspaceName as an empty string when RunRestoreTxn calls NewMgr().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@br/pkg/task/restore_txn.go` at line 33, runRestoreTxnCommand is not picking
up the --keyspace-name flag because task.Config.ParseFromFlags() doesn't read
FlagKeyspaceName; ensure cfg.KeyspaceName is populated before calling NewMgr by
reading the persistent flag added by DefineRestoreFlags. Fix by retrieving the
value of FlagKeyspaceName from the command flags (e.g.
cmd.Flags().GetString(FlagKeyspaceName) or cmd.PersistentFlags().GetString) or
by invoking the same parsing used by RestoreConfig.ParseFromFlags(), then assign
it into cfg.KeyspaceName so RunRestoreTxn and NewMgr receive the correct
keyspace value.
|
@lhy1024: 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. |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
It is from https://github.com/tidbcloud/tidb-cse/pull/521
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Improvements