lightning: fix pd http request using old address #45680

lichunzhu · 2023-07-31T02:46:56Z

What problem does this PR solve?

Issue Number: close #43436

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test

Side effects

None

Documentation

None

Release note

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

lightning: fix the problem that lightning can work if all pd in its address is scaled

tiprow · 2023-07-31T02:47:17Z

Hi @lichunzhu. 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/test-infra repository.

codecov · 2023-07-31T02:59:23Z

Codecov Report

Merging #45680 (52a4250) into master (cede736) will increase coverage by 0.0044%.
Report is 14 commits behind head on master.
The diff coverage is 10.8695%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #45680        +/-   ##
================================================
+ Coverage   73.2119%   73.2164%   +0.0044%     
================================================
  Files          1265       1269         +4     
  Lines        389939     391382      +1443     
================================================
+ Hits         285482     286556      +1074     
- Misses        86154      86441       +287     
- Partials      18303      18385        +82

Flag	Coverage Δ
integration	`78.1388% <ø> (?)`
unit	`73.2079% <10.8695%> (-0.0041%)`	⬇️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0359% <ø> (-0.0059%)`	⬇️
br	`52.0071% <14.7058%> (-0.0004%)`	⬇️

hawkingrei · 2023-07-31T03:05:01Z

/ok-to-test

D3Hunter · 2023-07-31T06:37:05Z

executor/importer/table_import.go

@@ -122,7 +123,13 @@ func GetTiKVModeSwitcher(logger *zap.Logger) (local.TiKVModeSwitcher, error) {
 	if err != nil {
 		return nil, err
 	}
-	return NewTiKVModeSwitcher(tls, tidbCfg.Path, logger), nil
+	tlsOpt := tls.ToPDSecurityOption()
+	pdCli, err := pd.NewClientWithContext(ctx, []string{tidbCfg.Path}, tlsOpt)


requires close

D3Hunter · 2023-07-31T06:38:04Z

br/pkg/lightning/importer/import.go

@@ -294,6 +295,10 @@ func NewImportControllerWithPauser(
 	if err != nil {
 		return nil, err
 	}
+	pdCli, err := pd.NewClientWithContext(ctx, []string{cfg.TiDB.PdAddr}, tls.ToPDSecurityOption())


not closed?

other places too, seems not close anywhere

already addressed in f163612
PTAL again

lance6716 · 2023-07-31T07:30:47Z

rest lgtm. Please address existing comments

lichunzhu · 2023-07-31T09:57:39Z

/test check-dev2

tiprow · 2023-07-31T09:58:29Z

@lichunzhu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev2

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/test-infra repository.

lichunzhu · 2023-07-31T11:20:26Z

/test check-dev2

tiprow · 2023-07-31T11:20:57Z

@lichunzhu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev2

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/test-infra repository.

tiprow · 2023-08-01T07:20:32Z

@lichunzhu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev2

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/test-infra repository.

lichunzhu · 2023-08-01T07:21:02Z

/retest

lichunzhu · 2023-08-01T07:28:32Z

/retest

lichunzhu · 2023-08-01T08:15:27Z

/retest

lance6716 · 2023-08-01T09:38:17Z

/lgtm

ti-chi-bot · 2023-08-01T09:38:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,lance6716]

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 · 2023-08-01T09:38:27Z

[LGTM Timeline notifier]

Timeline:

2023-08-01 06:31:17.354936762 +0000 UTC m=+166961.297285286: ☑️ agreed by D3Hunter.
2023-08-01 09:38:26.536072868 +0000 UTC m=+178190.478421391: ☑️ agreed by lance6716.

lichunzhu · 2023-08-01T10:07:45Z

/run-br-integration-tests

ti-chi-bot · 2023-08-01T11:22:52Z

In response to a cherrypick label: new pull request created to branch release-7.1: #45726.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-08-01T11:23:36Z

In response to a cherrypick label: new pull request created to branch release-6.5: #45728.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

lichunzhu · 2023-08-01T13:49:11Z

/cherry-pick release-7.3

ti-chi-bot · 2023-08-01T13:49:56Z

@lichunzhu: new pull request created to branch release-7.3: #45737.

In response to this:

/cherry-pick release-7.3

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 ti-community-infra/tichi repository.

close pingcap#43436

lichunzhu · 2023-08-01T13:57:03Z

/run-br-integration-test

close #43436

mittalrishabh · 2023-09-09T17:23:59Z

This PR seems incomplete to me. Why we are not using leader during scatter and split in client.go.

lance6716 · 2023-09-10T13:11:10Z

This PR seems incomplete to me. Why we are not using leader during scatter and split in client.go.

Hi, do you mean

tidb/br/pkg/restore/split/client.go

Lines 330 to 336 in d3d30f5

    
           func sendSplitRegionRequest(ctx context.Context, c *pdClient, regionInfo *RegionInfo, 
        
           	keys [][]byte, splitErrors *error, retry int) (bool, *kvrpcpb.SplitRegionResponse, error) { 
        
           	var peer *metapb.Peer 
        
           	// scanRegions may return empty Leader in https://github.com/tikv/pd/blob/v4.0.8/server/grpc_service.go#L524 
        
           	// so wee also need check Leader.Id != 0 
        
           	if regionInfo.Leader != nil && regionInfo.Leader.Id != 0 { 
        
           		peer = regionInfo.Leader

We have used the region leader from given regionInfo

mittalrishabh · 2023-09-10T16:42:33Z

This PR seems incomplete to me. Why we are not using leader during scatter and split in client.go.

Hi, do you mean

tidb/br/pkg/restore/split/client.go

Lines 330 to 336 in d3d30f5

func sendSplitRegionRequest(ctx context.Context, c *pdClient, regionInfo *RegionInfo,

keys [][]byte, splitErrors *error, retry int) (bool, *kvrpcpb.SplitRegionResponse, error) {

var peer *metapb.Peer

// scanRegions may return empty Leader in https://github.com/tikv/pd/blob/v4.0.8/server/grpc_service.go#L524

// so wee also need check Leader.Id != 0

if regionInfo.Leader != nil && regionInfo.Leader.Id != 0 {

peer = regionInfo.Leader

We have used the region leader from given regionInfo

Never mind. I misinterpreted. I suppose any grpc request through pd client goes to leader. This PR is for http request only. Is that right ?

lance6716 · 2023-09-11T02:26:32Z

Yes. The PD client interface https://github.com/tikv/pd/blob/d031342b4e104a3ba21812253ad84f5576aea994/client/client.go#L79 will internally choose current leader to send the request (or send to followers when network partition https://github.com/tikv/pd/blob/d031342b4e104a3ba21812253ad84f5576aea994/client/client.go#L804)

But in some usage we don't use the Client interface, for example, we want to peek the leader address and use HTTP API. That's the scope of this PR and #46726 .

And I have opened issue tikv/pd#7063 at PD repo to discuss about the usability of Client interface. Welcome to discuss in the new issue. This PR is closed and no other developers will notice our discussion.

close #43436

lichunzhu added 3 commits July 28, 2023 16:53

fix lightning http auto sync

9b77e00

fix bazel

ab2a95d

merge master and resolve conflicts

759f138

ti-chi-bot bot added release-note-none needs-cherry-pick-release-6.5 needs-cherry-pick-release-7.1 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2023

ti-chi-bot bot added ok-to-test release-note and removed release-note-none labels Jul 31, 2023

lichunzhu added 2 commits July 31, 2023 11:37

fix

b436b57

fix unit test

7db9ecb

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2023

fix bazel

bf57b6a

D3Hunter reviewed Jul 31, 2023

View reviewed changes

fix ut

4a88031

address comment

f163612

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2023

Merge branch 'master' into fixLightningAutoSync

721f71f

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2023

fix

5ff5a07

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels Aug 1, 2023

ti-chi-bot bot merged commit 9c213aa into pingcap:master Aug 1, 2023
16 of 22 checks passed

ti-chi-bot mentioned this pull request Aug 1, 2023

lightning: fix pd http request using old address (#45680) #45726

Merged

1 task

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 1, 2023

This is an automated cherry-pick of pingcap#45680

d57d450

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Aug 1, 2023

lightning: fix pd http request using old address (#45680) #45728

Merged

1 task

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 1, 2023

This is an automated cherry-pick of pingcap#45680

1f0038d

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Aug 1, 2023

lightning: fix pd http request using old address (#45680) #45737

Merged

1 task

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 1, 2023

lightning: fix pd http request using old address (pingcap#45680)

d240f00

close pingcap#43436

lichunzhu deleted the fixLightningAutoSync branch August 1, 2023 13:57

ti-chi-bot bot pushed a commit that referenced this pull request Aug 4, 2023

lightning: fix pd http request using old address (#45680) (#45737)

203d839

close #43436

ti-chi-bot bot pushed a commit that referenced this pull request Aug 16, 2023

lightning: fix pd http request using old address (#45680) (#45728)

415c36c

close #43436

ti-chi-bot bot pushed a commit that referenced this pull request Oct 17, 2023

lightning: fix pd http request using old address (#45680) (#45726)

acdac74

close #43436

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: fix pd http request using old address #45680

lightning: fix pd http request using old address #45680

lichunzhu commented Jul 31, 2023 •

edited

tiprow bot commented Jul 31, 2023

codecov bot commented Jul 31, 2023 •

edited

hawkingrei commented Jul 31, 2023

D3Hunter Jul 31, 2023

D3Hunter Jul 31, 2023

D3Hunter Jul 31, 2023

lichunzhu Jul 31, 2023

lance6716 commented Jul 31, 2023

lichunzhu commented Jul 31, 2023

tiprow bot commented Jul 31, 2023

lichunzhu commented Jul 31, 2023

tiprow bot commented Jul 31, 2023

tiprow bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lance6716 commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

mittalrishabh commented Sep 9, 2023

lance6716 commented Sep 10, 2023

mittalrishabh commented Sep 10, 2023 •

edited

lance6716 commented Sep 11, 2023 •

edited

lightning: fix pd http request using old address #45680

lightning: fix pd http request using old address #45680

Conversation

lichunzhu commented Jul 31, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Jul 31, 2023

codecov bot commented Jul 31, 2023 • edited

Codecov Report

hawkingrei commented Jul 31, 2023

D3Hunter Jul 31, 2023

Choose a reason for hiding this comment

D3Hunter Jul 31, 2023

Choose a reason for hiding this comment

D3Hunter Jul 31, 2023

Choose a reason for hiding this comment

lichunzhu Jul 31, 2023

Choose a reason for hiding this comment

lance6716 commented Jul 31, 2023

lichunzhu commented Jul 31, 2023

tiprow bot commented Jul 31, 2023

lichunzhu commented Jul 31, 2023

tiprow bot commented Jul 31, 2023

tiprow bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

lance6716 commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

[LGTM Timeline notifier]

lichunzhu commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

ti-chi-bot commented Aug 1, 2023

lichunzhu commented Aug 1, 2023

mittalrishabh commented Sep 9, 2023

lance6716 commented Sep 10, 2023

mittalrishabh commented Sep 10, 2023 • edited

lance6716 commented Sep 11, 2023 • edited

lichunzhu commented Jul 31, 2023 •

edited

codecov bot commented Jul 31, 2023 •

edited

mittalrishabh commented Sep 10, 2023 •

edited

lance6716 commented Sep 11, 2023 •

edited