Repository navigation
*: use meta service group, separate PD addrs from TiDB's required etcd addrs - #69152
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
|
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 metaservice-based PD discovery: EtcdBackend gains GetPDAddrs/MetaServiceInfo, TiKV driver and consumers are refactored to use metaservice info, infosync stores a metaservice client, and tests/mocks/BUILD deps are updated to match the new interface. ChangesMetaservice PD discovery rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Hi @ystaticy. 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 31-34: NewEtcdMetaServiceClient currently delegates to newClient
which treats a nil etcdCli as a hard failure, preventing PD-only setups from
exposing GetPDAddrs/GetPDHttpAddrs; change the construction so that
NewEtcdMetaServiceClient returns a usable ServiceClient when pdCli != nil even
if etcdCli == nil by updating newClient (or NewEtcdMetaServiceClient) to branch:
if etcdCli != nil build the full etcd+PD client as before, otherwise build a
PD-only client that implements the PD-related methods
(GetPDAddrs/GetPDHttpAddrs); ensure the PD-only client type satisfies
ServiceClient and that GlobalInfoSyncerInit can store it unconditionally.
In `@pkg/server/http_status.go`:
- Around line 591-598: The log message for the EtcdAddrs() error is
misleading—update the error log in the block handling "etcdAddr, err :=
ebd.EtcdAddrs()" to reflect that EtcdAddrs() failed (e.g., "failed to get etcd
addresses from EtcdBackend" or "EtcdAddrs() returned error") instead of "tikv
store not etcd backend"; keep the same conditional branch using intest.InTest
and use logutil.BgLogger().Panic / .Error with zap.Error(err) and the
ebd/EtcdAddrs() context.
In `@pkg/store/driver/tikv_driver.go`:
- Around line 257-266: OpenWithOptions currently writes a single metaServiceInfo
pointer into the tikvStore struct (metaServiceInfo) and
MetaServiceInfo()/EtcdAddrs() blindly return that cached pointer forever; change
this so the store does not freeze discovery: remove the long-lived cached
metaServiceInfo usage and instead store a meta service client/provider or a 작은
fetch function on tikvStore (e.g., metaServiceClient or metaServiceProvider) and
update MetaServiceInfo() to call that provider each time (or refresh on TTL) so
EtcdAddrs() always gets current endpoints; update OpenWithOptions to initialize
the client/provider rather than a static pointer and keep symbol references:
OpenWithOptions, tikvStore.metaServiceInfo, MetaServiceInfo(), and EtcdAddrs().
- Around line 221-229: The code dereferences metaServiceInfo.Group.Addrs without
checking for nil after calling metaservice.GetInfo, which can panic when no
group is resolved; update the block after metaservice.GetInfo to guard that
metaServiceInfo and metaServiceInfo.Group are non-nil and return a clear error
if missing before calling NewEtcdSafePointKVWithKeyspacePrefixIfNeeded (refer to
metaservice.GetInfo, metaServiceInfo.Group.Addrs,
NewEtcdSafePointKVWithKeyspacePrefixIfNeeded, and spkv); ensure the error
message references the keyspace (codec.GetKeyspaceMeta()) to make the failure
actionable.
In `@pkg/store/etcd_test.go`:
- Around line 36-47: The mockEtcdBackend stub returns the same slice for PD and
metaservice endpoints which hides regressions; change
mockEtcdBackend.GetPDAddrs() to return a pdAddrs slice and have
MetaServiceInfo() populate Group.Addrs with a distinct metaAddrs slice (both
stored on mockEtcdBackend fields) so tests can distinguish them, then update the
test assertion to call GetEtcdAddrs() and assert it returns metaAddrs (not
pdAddrs); update any references to pdAddrs/metaAddrs within
pkg/store/etcd_test.go accordingly.
🪄 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: 81e2ab97-bbc7-464e-a441-08d529dcede9
📒 Files selected for processing (32)
lightning/pkg/importer/BUILD.bazellightning/pkg/importer/import.gopkg/ddl/BUILD.bazelpkg/ddl/integration_test.gopkg/ddl/owner_mgr_test.gopkg/domain/BUILD.bazelpkg/domain/domain_test.gopkg/domain/infosync/BUILD.bazelpkg/domain/infosync/info.gopkg/executor/BUILD.bazelpkg/executor/hot_regions_history_table_test.gopkg/executor/infoschema_cluster_table_test.gopkg/executor/split.gopkg/infoschema/tables.gopkg/kv/BUILD.bazelpkg/kv/kv.gopkg/metaservice/etcd.gopkg/server/BUILD.bazelpkg/server/handler/tikvhandler/tikv_handler.gopkg/server/http_status.gopkg/server/server_test.gopkg/session/BUILD.bazelpkg/session/bootstrap_test.gopkg/store/BUILD.bazelpkg/store/driver/BUILD.bazelpkg/store/driver/tikv_driver.gopkg/store/etcd_test.gopkg/store/helper/BUILD.bazelpkg/store/helper/helper.gopkg/store/helper/helper_test.gopkg/store/mockstore/mockstorage/BUILD.bazelpkg/store/mockstore/mockstorage/storage.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69152 +/- ##
================================================
- Coverage 76.3204% 76.1048% -0.2157%
================================================
Files 2041 2072 +31
Lines 561097 577803 +16706
================================================
+ Hits 428232 439736 +11504
- Misses 131964 136063 +4099
- Partials 901 2004 +1103
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>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
Signed-off-by: ystaticy <y_static_y@sina.com>
|
rest lgtm |
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
|
/test pull-br-integration-test |
1 similar comment
|
/test pull-br-integration-test |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, YangKeao, YuJuncen 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 |
Signed-off-by: ystaticy <y_static_y@sina.com>
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
Refactor
Tests