*: br,lightning,import use meta service group#69780
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
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:
📝 WalkthroughWalkthroughBR, Lightning, executor importer, and store etcd paths now resolve metaservice endpoints and keyspace namespaces from PD metadata. Shared utilities centralize endpoint parsing and namespaced client creation, with integration tests covering Unix endpoints and keyspace-scoped writes. ChangesKeyspace-aware etcd resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as BR or Lightning caller
participant Meta as metaservice.DialEtcdClient
participant PD as PD client
participant Etcd as etcd client
Caller->>Meta: Request etcd client with keyspace name
Meta->>PD: Load keyspace metadata and members
Meta->>Meta: Resolve endpoints and namespace
Meta->>Etcd: Create namespaced client
Meta-->>Caller: Return configured etcd client
Caller->>Etcd: Write keyspace-scoped task data
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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.
🧹 Nitpick comments (1)
pkg/store/etcd_test.go (1)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
metaservice.GroupIDKeyandmetaservice.GroupAddrsKeyconstants instead of string literals.The Lightning test (
lightning/pkg/importer/meta_service_group_test.go:64-65) uses themetaservicepackage constants for these config keys, while this test uses raw string literals. If the constant values ever change, this test would break silently. Using the constants would require adding//pkg/metaserviceto thestore_testBUILD.bazel deps.♻️ Optional refactor for consistency
Config: map[string]string{ "gc_management_type": "keyspace_level", - "meta_service_group_id": "group1", - "meta_service_group_addrs": strings.Join(metaCluster.Client(0).Endpoints(), ","), + metaservice.GroupIDKey: "group1", + metaservice.GroupAddrsKey: strings.Join(metaCluster.Client(0).Endpoints(), ","), }This would also require adding the import and BUILD.bazel dependency:
+ "github.com/pingcap/tidb/pkg/metaservice"In
pkg/store/BUILD.bazel:+ "//pkg/metaservice",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/etcd_test.go` around lines 83 - 84, Replace the raw "meta_service_group_id" and "meta_service_group_addrs" keys in the test configuration with metaservice.GroupIDKey and metaservice.GroupAddrsKey. Add the metaservice import and declare the corresponding //pkg/metaservice dependency in the store_test BUILD.bazel target.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/store/etcd_test.go`:
- Around line 83-84: Replace the raw "meta_service_group_id" and
"meta_service_group_addrs" keys in the test configuration with
metaservice.GroupIDKey and metaservice.GroupAddrsKey. Add the metaservice import
and declare the corresponding //pkg/metaservice dependency in the store_test
BUILD.bazel target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1b1a3e6e-bb12-4902-80ab-d18c8dda4ab3
📒 Files selected for processing (10)
br/pkg/task/BUILD.bazelbr/pkg/task/common.gobr/pkg/task/meta_service_group_test.golightning/pkg/importer/BUILD.bazellightning/pkg/importer/meta_service_group_test.golightning/pkg/importer/precheck_impl.gopkg/metaservice/BUILD.bazelpkg/metaservice/etcd.gopkg/store/BUILD.bazelpkg/store/etcd_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69780 +/- ##
================================================
+ Coverage 76.3194% 76.3634% +0.0439%
================================================
Files 2041 2042 +1
Lines 560102 562363 +2261
================================================
+ Hits 427467 429440 +1973
- Misses 131734 131797 +63
- Partials 901 1126 +225
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
br/pkg/task/common.go (1)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider updating the log message to reflect post-connection state.
With
grpc.WithBlock()in the dial options,DialEtcdClientblocks until the etcd connection is established. By the time line 222 executes, the connection is already active, so "trying to connect to etcd" is slightly misleading. Consider "connected to etcd" for accuracy.✏️ Proposed tweak
- log.Info("trying to connect to etcd", zap.Strings("addr", etcdCLI.Endpoints())) + log.Info("connected to etcd", zap.Strings("addr", etcdCLI.Endpoints()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/task/common.go` at line 222, Update the log message in DialEtcdClient after the blocking etcd dial to state that the client is connected rather than still trying to connect, while preserving the existing endpoint fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/metaservice/etcd.go`:
- Around line 77-126: Update the missing-metadata error in DialEtcdClient to
include the keyspaceName value, while preserving the existing error path and
message meaning.
---
Nitpick comments:
In `@br/pkg/task/common.go`:
- Line 222: Update the log message in DialEtcdClient after the blocking etcd
dial to state that the client is connected rather than still trying to connect,
while preserving the existing endpoint fields.
🪄 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: 78a32d2f-21e3-4bf3-899e-1665759aa001
📒 Files selected for processing (9)
br/pkg/task/BUILD.bazelbr/pkg/task/common.gobr/pkg/task/meta_service_group_test.golightning/pkg/importer/BUILD.bazellightning/pkg/importer/meta_service_group_test.golightning/pkg/importer/precheck_impl.gopkg/metaservice/BUILD.bazelpkg/metaservice/etcd.gopkg/store/etcd_test.go
💤 Files with no reviewable changes (2)
- lightning/pkg/importer/BUILD.bazel
- br/pkg/task/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (4)
- lightning/pkg/importer/meta_service_group_test.go
- br/pkg/task/meta_service_group_test.go
- pkg/store/etcd_test.go
- lightning/pkg/importer/precheck_impl.go
| // DialEtcdClient resolves the target meta service group and returns a namespaced etcd client. | ||
| func DialEtcdClient( | ||
| ctx context.Context, | ||
| keyspaceName string, | ||
| pdAddrs []string, | ||
| security pd.SecurityOption, | ||
| pdClientFactory PDClientFactory, | ||
| callerComponent caller.Component, | ||
| pdClientOpts []opt.ClientOption, | ||
| etcdCfg clientv3.Config, | ||
| ) (*clientv3.Client, error) { | ||
| if pdClientFactory == nil { | ||
| pdClientFactory = pd.NewClientWithAPIContext | ||
| } | ||
|
|
||
| pdCli, err := pdClientFactory( | ||
| ctx, keyspace.BuildAPIContext(keyspaceName), callerComponent, pdAddrs, security, pdClientOpts..., | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer pdCli.Close() | ||
|
|
||
| var keyspaceMeta *keyspacepb.KeyspaceMeta | ||
| if keyspaceName != "" { | ||
| keyspaceMeta, err = pdCli.LoadKeyspace(ctx, keyspaceName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if keyspaceMeta == nil { | ||
| return nil, errors.New("keyspace meta not found") | ||
| } | ||
| } | ||
|
|
||
| dialInfo, err := ResolveEtcdDialInfo(ctx, pdCli, keyspaceMeta) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| etcdCfg.Context = ctx | ||
| etcdCfg.Endpoints = dialInfo.Endpoints | ||
| etcdCli, err := clientv3.New(etcdCfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if dialInfo.Namespace != "" { | ||
| etcd.SetEtcdCliByNamespace(etcdCli, dialInfo.Namespace) | ||
| } | ||
| return etcdCli, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include keyspace name in the "not found" error message.
The error at line 107 (errors.New("keyspace meta not found")) is actionable but lacks context — when multiple keyspaces are in play, the operator cannot tell which keyspace failed to resolve. Adding the keyspace name makes triage significantly easier.
🛡️ Proposed fix
if keyspaceMeta == nil {
- return nil, errors.New("keyspace meta not found")
+ return nil, errors.Errorf("keyspace meta not found for keyspace %q", keyspaceName)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DialEtcdClient resolves the target meta service group and returns a namespaced etcd client. | |
| func DialEtcdClient( | |
| ctx context.Context, | |
| keyspaceName string, | |
| pdAddrs []string, | |
| security pd.SecurityOption, | |
| pdClientFactory PDClientFactory, | |
| callerComponent caller.Component, | |
| pdClientOpts []opt.ClientOption, | |
| etcdCfg clientv3.Config, | |
| ) (*clientv3.Client, error) { | |
| if pdClientFactory == nil { | |
| pdClientFactory = pd.NewClientWithAPIContext | |
| } | |
| pdCli, err := pdClientFactory( | |
| ctx, keyspace.BuildAPIContext(keyspaceName), callerComponent, pdAddrs, security, pdClientOpts..., | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer pdCli.Close() | |
| var keyspaceMeta *keyspacepb.KeyspaceMeta | |
| if keyspaceName != "" { | |
| keyspaceMeta, err = pdCli.LoadKeyspace(ctx, keyspaceName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if keyspaceMeta == nil { | |
| return nil, errors.New("keyspace meta not found") | |
| } | |
| } | |
| dialInfo, err := ResolveEtcdDialInfo(ctx, pdCli, keyspaceMeta) | |
| if err != nil { | |
| return nil, err | |
| } | |
| etcdCfg.Context = ctx | |
| etcdCfg.Endpoints = dialInfo.Endpoints | |
| etcdCli, err := clientv3.New(etcdCfg) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if dialInfo.Namespace != "" { | |
| etcd.SetEtcdCliByNamespace(etcdCli, dialInfo.Namespace) | |
| } | |
| return etcdCli, nil | |
| } | |
| if keyspaceMeta == nil { | |
| return nil, errors.Errorf("keyspace meta not found for keyspace %q", keyspaceName) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metaservice/etcd.go` around lines 77 - 126, Update the missing-metadata
error in DialEtcdClient to include the keyspaceName value, while preserving the
existing error path and message meaning.
|
/retest-required |
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XXL
|
[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 |
|
/retest-required |
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
Signed-off-by: ystaticy <y_static_y@sina.com>
|
@ystaticy: The following tests 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/metaservice/etcd.go`:
- Around line 194-202: Update GetPDAddrs so malformed ClientUrls entries are
skipped rather than returned as errors: continue iterating members when
util.ParseServiceURL fails, while preserving valid endpoints. Keep the existing
len(pdAddrs) == 0 fallback/error behavior so the call succeeds only when at
least one usable address is found.
🪄 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: 50b2f62f-5cd7-4642-ac45-63c350000b30
📒 Files selected for processing (8)
pkg/metaservice/BUILD.bazelpkg/metaservice/etcd.gopkg/store/mockstore/unistore/BUILD.bazelpkg/store/mockstore/unistore/pd.gopkg/store/mockstore/unistore/pd_test.gopkg/util/BUILD.bazelpkg/util/service_url.gopkg/util/service_url_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/store/mockstore/unistore/BUILD.bazel
- pkg/store/mockstore/unistore/pd_test.go
- pkg/metaservice/BUILD.bazel
- pkg/store/mockstore/unistore/pd.go
| for _, member := range members.GetMembers() { | ||
| if len(member.ClientUrls) > 0 { | ||
| prefix, hostPort, err := ParseURL(member.ClientUrls[0]) | ||
| endpoint, err := util.ParseServiceURL(member.ClientUrls[0]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parse client url from pd members %q: %w", member.ClientUrls[0], err) | ||
| } | ||
| var pdAddr string | ||
| if withSchema { | ||
| pdAddr = prefix + hostPort // http://ip:port | ||
| } else { | ||
| pdAddr = hostPort // ip:port | ||
| } | ||
|
|
||
| pdAddrs = append(pdAddrs, pdAddr) | ||
| pdAddrs = append(pdAddrs, endpoint.Endpoint(withSchema)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Single malformed PD member URL aborts the whole GetPDAddrs call.
A parse failure on one member's ClientUrls[0] returns immediately, even though the trailing len(pdAddrs) == 0 check (lines 203-205) implies the intended contract is "succeed if at least one member yields a usable URL." This is stricter than the mock counterpart, normalizeMockPDAddrs in pkg/store/mockstore/unistore/pd.go, which calls util.NormalizeServiceURL with util.URLSchemeHTTP and drops entries that fail normalization instead of failing outright. A single member reporting a malformed ClientUrls[0] would now break etcd dialing for BR/Lightning/executor-importer even if other members are healthy.
🛡️ Proposed fix to skip malformed entries instead of failing outright
for _, member := range members.GetMembers() {
if len(member.ClientUrls) > 0 {
endpoint, err := util.ParseServiceURL(member.ClientUrls[0])
if err != nil {
- return nil, fmt.Errorf("parse client url from pd members %q: %w", member.ClientUrls[0], err)
+ // Skip this member; other members may still provide a usable client URL,
+ // matching the len(pdAddrs) == 0 fallback check below.
+ continue
}
pdAddrs = append(pdAddrs, endpoint.Endpoint(withSchema))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, member := range members.GetMembers() { | |
| if len(member.ClientUrls) > 0 { | |
| prefix, hostPort, err := ParseURL(member.ClientUrls[0]) | |
| endpoint, err := util.ParseServiceURL(member.ClientUrls[0]) | |
| if err != nil { | |
| return nil, fmt.Errorf("parse client url from pd members %q: %w", member.ClientUrls[0], err) | |
| } | |
| var pdAddr string | |
| if withSchema { | |
| pdAddr = prefix + hostPort // http://ip:port | |
| } else { | |
| pdAddr = hostPort // ip:port | |
| } | |
| pdAddrs = append(pdAddrs, pdAddr) | |
| pdAddrs = append(pdAddrs, endpoint.Endpoint(withSchema)) | |
| } | |
| } | |
| for _, member := range members.GetMembers() { | |
| if len(member.ClientUrls) > 0 { | |
| endpoint, err := util.ParseServiceURL(member.ClientUrls[0]) | |
| if err != nil { | |
| // Skip this member; other members may still provide a usable client URL, | |
| // matching the len(pdAddrs) == 0 fallback check below. | |
| continue | |
| } | |
| pdAddrs = append(pdAddrs, endpoint.Endpoint(withSchema)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metaservice/etcd.go` around lines 194 - 202, Update GetPDAddrs so
malformed ClientUrls entries are skipped rather than returned as errors:
continue iterating members when util.ParseServiceURL fails, while preserving
valid endpoints. Keep the existing len(pdAddrs) == 0 fallback/error behavior so
the call succeeds only when at least one usable address is found.
Source: Linked repositories
| }) | ||
| } | ||
| return metaservice.DialEtcdClient( | ||
| ctx, cfg.TikvImporter.KeyspaceName, addrs, tlsCfg.ToPDSecurityOption(), |
There was a problem hiding this comment.
This precheck should use the runtime keyspace passed through ControllerParam.KeyspaceName, rather than cfg.TikvImporter.KeyspaceName.
NewImportControllerWithPauser constructs the PD client, codec, local backend, and Controller using p.KeyspaceName, but NewPrecheckItemBuilder does not receive that value. Therefore, an embedded caller can import into one keyspace while the CDC/PiTR precheck connects to the global group or a different keyspace configured in cfg.TikvImporter.KeyspaceName.
In that case, the precheck may incorrectly report that no CDC or PiTR task exists and allow a local-backend import that should have been rejected.
Could we pass p.KeyspaceName through PrecheckItemBuilder and CDCPITRCheckItem, or explicitly validate and synchronize these two values? Please also add a test where ControllerParam.KeyspaceName is non-empty but cfg.TikvImporter.KeyspaceName is empty or different.
|
|
||
| // ResolveEtcdDialInfo resolves the etcd endpoints and namespace for a keyspace. | ||
| func ResolveEtcdDialInfo(ctx context.Context, pdCli pd.Client, keyspaceMeta *keyspacepb.KeyspaceMeta) (EtcdDialInfo, error) { | ||
| _, endpoints, err := GetInfoAndGroupAddrs(ctx, pdCli, keyspaceMeta) |
There was a problem hiding this comment.
Calling GetInfoAndGroupAddrs here makes dedicated meta-service-group resolution unnecessarily depend on the PD members' advertised client URLs.
GetInfoAndGroupAddrs calls MetaServiceManager.FetchInfo, which first invokes GetPDAddrs and only then reads the dedicated group information from keyspaceMeta. As a result, even when keyspaceMeta already contains valid meta_service_group_id and meta_service_group_addrs, this function can still fail because a PD member has missing, malformed, or unreachable advertised client URLs.
This is especially problematic for deployments where the caller connects to PD through a load balancer, proxy, NAT, or Kubernetes Service, while the advertised member addresses are not directly reachable. It may also change the global-group fallback behavior by replacing the caller-provided endpoints with PD-advertised addresses. Additionally, GetPDAddrs currently only uses member.ClientUrls[0], so a valid secondary URL is ignored.
Could we avoid calling GetInfoAndGroupAddrs for this purpose and resolve the dedicated group directly from keyspaceMeta? For the global-group fallback, it would be safer to preserve the caller-provided endpoints. If advertised URLs are still required, all ClientUrls should be considered rather than only the first one.
2 participants
What problem does this PR solve?
Issue Number: ref #68338
Problem Summary:
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
Summary of updates
New Features
Bug Fixes
unix://endpoints for metadata-driven dialing.Tests
Chores