lightning: add precheck that PD and TiDB must belongs to same cluster #57709

lance6716 · 2024-11-26T07:37:49Z

What problem does this PR solve?

Issue Number: close #57704

Problem Summary:

What changed and how does it work?

lance6716
summarize the file changed of this PR. No need to mention the details like filename or number of lines changed, just from logical and abstract view to write a summary.

Copilot
The changes in PR #57709 for the pingcap/tidb repository include adding a precheck function to ensure that PD and TiDB belong to the same cluster. This new validation logic is integrated into the existing precheck mechanisms within the lightning package.

Check List

Tests

Unit test
Integration test: CI failed when I forgot to strip the URL scheme
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: lance6716 <lance6716@gmail.com>

tiprow · 2024-11-26T07:38:05Z

Hi @lance6716. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions 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.

Signed-off-by: lance6716 <lance6716@gmail.com>

codecov · 2024-11-26T08:47:04Z

Codecov Report

Attention: Patch coverage is 61.11111% with 35 lines in your changes missing coverage. Please review.

Project coverage is 73.7008%. Comparing base (d02d48a) to head (d7a8dfb).
Report is 16 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57709        +/-   ##
================================================
+ Coverage   72.8318%   73.7008%   +0.8690%     
================================================
  Files          1677       1707        +30     
  Lines        463944     476191     +12247     
================================================
+ Hits         337899     350957     +13058     
+ Misses       105169     103631      -1538     
- Partials      20876      21603       +727

Flag	Coverage Δ
integration	`46.1800% <36.6666%> (?)`
unit	`72.2268% <59.4594%> (+0.0029%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.1334% <ø> (-0.3779%)`	⬇️

lance6716 · 2024-11-26T10:23:43Z

/retest

tiprow · 2024-11-26T10:24:15Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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.

lance6716 · 2024-11-26T12:53:17Z

/retest

tiprow · 2024-11-26T12:53:44Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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.

wuhuizuo · 2024-11-26T13:35:37Z

/retest

tiprow · 2024-11-26T13:36:04Z

@wuhuizuo: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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.

Signed-off-by: lance6716 <lance6716@gmail.com>

D3Hunter · 2024-11-27T06:36:27Z

lightning/pkg/importer/precheck_impl.go

+			return nil, errors.Trace(err)
+		}
+
+		// if intersection is not empty, we can say URLs from TiDB and PD are from the same cluster


please comment that there will be false positive if user configs a LB address

PD addresses read from PD or TiDB are from the same source, which is the etcd member's ClientURL. If PD has multiple reachable endpoints on network, it will not cause false positive. I will add comment to explain the source of addresses in next commit

if it fails to get, will return user configured address

pdAddrsGetter = func(ctx context.Context) []string { leaderInfo, err := pdHTTPCli.GetLeader(ctx) if err != nil { return []string{cfg.TiDB.PdAddr} } addrs := leaderInfo.GetClientUrls() if len(addrs) == 0 { return []string{cfg.TiDB.PdAddr} }

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2024-11-27T07:25:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [D3Hunter,tangenta]
~~lightning/OWNERS~~ [D3Hunter]
~~pkg/lightning/OWNERS~~ [D3Hunter]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-11-27T07:25:39Z

[LGTM Timeline notifier]

Timeline:

2024-11-27 06:44:31.439505657 +0000 UTC m=+618859.059160172: ☑️ agreed by D3Hunter.
2024-11-27 07:25:38.562844839 +0000 UTC m=+621326.182499353: ☑️ agreed by tangenta.

lance6716 · 2024-11-27T07:37:53Z

/hold

wait for CDC reuse our logic

lance6716 · 2024-11-27T07:39:45Z

/unhold

wlwilliamx · 2024-11-27T07:43:20Z

/hold
wait for CDC to reuse

wlwilliamx · 2024-11-27T07:35:13Z

lightning/pkg/importer/precheck_impl.go

+	db            *sql.DB
+	pdAddrsGetter func(context.Context) []string


I recommend replacing these fields in the struct like:

getPDAddrsFromSQL func(context.Context) []string getPDAddrsFromPD func(context.Context) []string

This change improves flexibility and provides a clearer abstraction by distinguishing between two different PD address getters(and it's better for CDC to use in upstream and downstream). Additionally, it simplifies unit testing, as you can now mock and test the two getters independently.

distinguishing between upstream and downstream PD address

There's no upstream PD addresses. However I understand your meaning is check the two ways to get PD addresses.

I don't know how CDC can reuse this struct pdTiDBFromSameClusterCheckItem, because the intersection assertion can not be configurated, and CDC wants to assert non-intersection.

Signed-off-by: lance6716 <lance6716@gmail.com>

wlwilliamx · 2024-11-27T10:45:34Z

pkg/util/util.go

+			return true, addrs, addrs2, nil
+		}
+	}
+	return false, addrs, addrs2, nil


We can add a log to print addrs and addrs2. And it's no need to return these addrs.

wlwilliamx · 2024-11-27T10:50:57Z

LGTM

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2024-11-27T12:32:15Z

/unhold

lightning: add precheck that PD and TiDB must belongs to same cluster

7b353a0

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2024

lance6716 added 2 commits November 26, 2024 16:21

fix CI

b62c228

Signed-off-by: lance6716 <lance6716@gmail.com>

fix CI

f8acc4d

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'fix57704' of github.com:lance6716/tidb into fix57704

544007a

lance6716 added 2 commits November 26, 2024 22:32

Trigger Build

6935ea7

fix CI

05a10c8

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 changed the title ~~[WIP]lightning: add precheck that PD and TiDB must belongs to same cluster~~ lightning: add precheck that PD and TiDB must belongs to same cluster Nov 27, 2024

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2024

add UT

5b29952

Signed-off-by: lance6716 <lance6716@gmail.com>

D3Hunter approved these changes Nov 27, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 27, 2024

add comment

222a2ed

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta approved these changes Nov 27, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 27, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2024

wlwilliamx reviewed Nov 27, 2024

View reviewed changes

provide common functions for CDC

b7a0c37

Signed-off-by: lance6716 <lance6716@gmail.com>

wlwilliamx reviewed Nov 27, 2024

View reviewed changes

fix

d7a8dfb

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2024

ti-chi-bot bot merged commit ca395fa into pingcap:master Nov 27, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: add precheck that PD and TiDB must belongs to same cluster #57709

lightning: add precheck that PD and TiDB must belongs to same cluster #57709

lance6716 commented Nov 26, 2024 •

edited

Loading

tiprow bot commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

lance6716 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

lance6716 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

wuhuizuo commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

D3Hunter Nov 27, 2024

lance6716 Nov 27, 2024

D3Hunter Nov 27, 2024

lance6716 Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

lance6716 commented Nov 27, 2024

lance6716 commented Nov 27, 2024

wlwilliamx commented Nov 27, 2024

wlwilliamx Nov 27, 2024 •

edited

Loading

lance6716 Nov 27, 2024 •

edited

Loading

wlwilliamx Nov 27, 2024

wlwilliamx commented Nov 27, 2024

lance6716 commented Nov 27, 2024

lightning: add precheck that PD and TiDB must belongs to same cluster #57709

lightning: add precheck that PD and TiDB must belongs to same cluster #57709

Conversation

lance6716 commented Nov 26, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 26, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

lance6716 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

lance6716 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

wuhuizuo commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

D3Hunter Nov 27, 2024

Choose a reason for hiding this comment

lance6716 Nov 27, 2024

Choose a reason for hiding this comment

D3Hunter Nov 27, 2024

Choose a reason for hiding this comment

lance6716 Nov 27, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

[LGTM Timeline notifier]

lance6716 commented Nov 27, 2024

lance6716 commented Nov 27, 2024

wlwilliamx commented Nov 27, 2024

wlwilliamx Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

lance6716 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

wlwilliamx Nov 27, 2024

Choose a reason for hiding this comment

wlwilliamx commented Nov 27, 2024

lance6716 commented Nov 27, 2024

lance6716 commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

wlwilliamx Nov 27, 2024 •

edited

Loading

lance6716 Nov 27, 2024 •

edited

Loading