*: 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBR, Lightning, executor importer, and store etcd paths now resolve metaservice endpoints and keyspace namespaces from PD metadata. Shared metaservice APIs centralize this behavior, with integration tests covering namespaced 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 PD as PD client
participant Meta as metaservice.DialEtcdClient
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-->>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% 75.8433% -0.4762%
================================================
Files 2041 2082 +41
Lines 560102 580635 +20533
================================================
+ Hits 427467 440373 +12906
- Misses 131734 138133 +6399
- Partials 901 2129 +1228
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 |
|
@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. |
1 participant
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
Tests
Chores