*: 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
|
[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 |
ti-chi-bot
Bot
added
the
size/XL
📝 WalkthroughWalkthroughBR and Lightning etcd dialing now uses PD keyspace metadata to resolve metaservice endpoints and namespaces. Metaservice exposes the resolution API, while integration tests verify namespaced writes in BR, Lightning, and store packages. ChangesKeyspace-aware etcd resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Task as dialEtcdWithCfg
participant PD as PD client
participant Meta as ResolveEtcdDialInfo
participant Etcd as etcd client
Task->>PD: Load keyspace metadata
Task->>Meta: Resolve endpoints and namespace
Meta-->>Task: Return EtcdDialInfo
Task->>Etcd: Connect to resolved endpoints
Task->>Etcd: Apply keyspace namespace
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)
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.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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% 75.8561% -0.4634%
================================================
Files 2041 2082 +41
Lines 560102 580342 +20240
================================================
+ Hits 427467 440225 +12758
- Misses 131734 137995 +6261
- Partials 901 2122 +1221
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>
|
[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
🧹 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.
1 participant
What problem does this PR solve?
Issue Number: close #xxx
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