infosync: integrate PD HTTP client into the label manager #48738

JmPotato · 2023-11-21T07:09:24Z

What problem does this PR solve?

Issue Number: ref #35319, tikv/pd#7300

Problem Summary:

Integrate PD HTTP client into the label manager.

What is changed and how it works?

Replace the etcd client inside LabelRuleManager with PD HTTP client.
Reuse the type definitions within the PD SDK as much as possible.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Release note

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

None

codecov · 2023-11-21T07:26:57Z

Codecov Report

Merging #48738 (fb36395) into master (9062834) will increase coverage by 1.7752%.
Report is 18 commits behind head on master.
The diff coverage is 55.2083%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #48738        +/-   ##
================================================
+ Coverage   71.0864%   72.8617%   +1.7752%     
================================================
  Files          1365       1392        +27     
  Lines        404572     413041      +8469     
================================================
+ Hits         287596     300949     +13353     
+ Misses        97010      93148      -3862     
+ Partials      19966      18944      -1022

Flag	Coverage Δ
integration	`43.7541% <40.6250%> (?)`
unit	`71.2905% <55.2083%> (+0.2041%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.0092% <ø> (-3.9652%)`	⬇️

JmPotato · 2023-11-21T13:01:40Z

pkg/server/handler/tests/http_handler_test.go

- tikv.WithPDHTTPClient([]string{ts.Addr()}),
- ),
- )
+ ts.store, err = mockstore.NewMockStore()


The previous commit made changes to this code section, but it was later determined to be unnecessary during testing. This PR reverts those changes.

0f35b6f#diff-69c54bd9e8a29f957a0cb3d46941079117eadecfa6090abfac3bd05f78ec37e1L453

Signed-off-by: JmPotato <ghzpotato@gmail.com>

tiprow · 2023-11-22T05:55:56Z

@JmPotato: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`fb36395`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

JmPotato · 2023-11-22T06:00:50Z

/test check-dev2

tiprow · 2023-11-22T06:00:56Z

@JmPotato: 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.

JmPotato · 2023-11-22T06:33:42Z

/test unit-test

tiprow · 2023-11-22T06:33:47Z

@JmPotato: 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 unit-test

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.

JmPotato · 2023-11-22T08:42:58Z

PTAL

/cc @tangenta @zimulala

zimulala · 2023-11-23T03:13:34Z

pkg/domain/infosync/info.go

@@ -209,7 +209,10 @@ func GlobalInfoSyncerInit(
 if err != nil {
 return nil, err
 }
- is.labelRuleManager = initLabelRuleManager(etcdCli)
+ if pdHTTPCli != nil {
+ pdHTTPCli = pdHTTPCli.WithRespHandler(pdResponseHandler)


Could we put it to initLabelRuleManager? It seems we use pdHTTPCli only in initLabelRuleManager.

Later we will integrate the PD HTTP client into the Placement Rule manager below also, so I keep it initialized here.

zimulala · 2023-11-23T03:40:42Z

pkg/ddl/label/rule.go

+func NewRulePatch(setRules []*Rule, deleteRules []string) *pd.LabelRulePatch {
+ labelRules := make([]*pd.LabelRule, 0, len(setRules))
+ for _, rule := range setRules {
+ labelRules = append(labelRules, (*pd.LabelRule)(rule))


Does changing the type of setRules(line157) to pd.LabelRule also eliminate the need for conversions?

The main consideration here is that TiDB has made additional extensions to the SQL layer concept for the Rule type internally. Therefore, I decided to keep the relevant implementation of the Rule type in TiDB and hide the type conversion in the lower layer from the caller. Otherwise, we would have needed to completely replace Rule with pd.LabelRule, or constantly introduce type conversions throughout. However, prioritizing code readability over a slight performance sacrifice seems like a better choice.

zimulala

LGTM

ti-chi-bot · 2023-11-23T04:09:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, zimulala

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~~ [hawkingrei,zimulala]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/domain/OWNERS~~ [zimulala]

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-11-23T04:09:35Z

[LGTM Timeline notifier]

Timeline:

2023-11-22 02:15:56.939354433 +0000 UTC m=+370585.604580629: ☑️ agreed by hawkingrei.
2023-11-23 04:09:34.432978852 +0000 UTC m=+463803.098205046: ☑️ agreed by zimulala.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 21, 2023

JmPotato mentioned this pull request Nov 21, 2023

Tracking issue for the HTTP SDK tikv/pd#7300

Open

7 tasks

JmPotato force-pushed the adopt_pd_http_cli_to_infosyncer branch 2 times, most recently from 6d64793 to 7a7796b Compare November 21, 2023 09:46

JmPotato commented Nov 21, 2023

View reviewed changes

hawkingrei approved these changes Nov 22, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 22, 2023

JmPotato added 4 commits November 22, 2023 13:40

Integrate PD HTTP client into the label manager

48f34b1

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Fix PD HTTP client WithRespHandler

76d5266

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Fix the HTTP handler tests

e6bed42

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Check bodyBytes before unmarshal

fb36395

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato force-pushed the adopt_pd_http_cli_to_infosyncer branch from 7a7796b to fb36395 Compare November 22, 2023 05:40

ti-chi-bot bot requested review from tangenta and zimulala November 22, 2023 08:43

zimulala reviewed Nov 23, 2023

View reviewed changes

zimulala approved these changes Nov 23, 2023

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 23, 2023

ti-chi-bot bot merged commit 7e8de2f into pingcap:master Nov 23, 2023
15 of 16 checks passed

JmPotato deleted the adopt_pd_http_cli_to_infosyncer branch November 23, 2023 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infosync: integrate PD HTTP client into the label manager #48738

infosync: integrate PD HTTP client into the label manager #48738

JmPotato commented Nov 21, 2023

codecov bot commented Nov 21, 2023 •

edited

Loading

JmPotato Nov 21, 2023 •

edited

Loading

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

zimulala Nov 23, 2023

JmPotato Nov 23, 2023

zimulala Nov 23, 2023

JmPotato Nov 23, 2023

zimulala left a comment

ti-chi-bot bot commented Nov 23, 2023

ti-chi-bot bot commented Nov 23, 2023

infosync: integrate PD HTTP client into the label manager #48738

infosync: integrate PD HTTP client into the label manager #48738

Conversation

JmPotato commented Nov 21, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

JmPotato Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

tiprow bot commented Nov 22, 2023

JmPotato commented Nov 22, 2023

zimulala Nov 23, 2023

Choose a reason for hiding this comment

JmPotato Nov 23, 2023

Choose a reason for hiding this comment

zimulala Nov 23, 2023

Choose a reason for hiding this comment

JmPotato Nov 23, 2023

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 23, 2023

ti-chi-bot bot commented Nov 23, 2023

[LGTM Timeline notifier]

codecov bot commented Nov 21, 2023 •

edited

Loading

JmPotato Nov 21, 2023 •

edited

Loading