client: introduce the HTTP client #7304

JmPotato · 2023-11-02T09:21:46Z

What problem does this PR solve?

Issue Number: ref #7300.

What is changed and how does it work?

Introduce the HTTP client.

Check List

Tests

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

Release note

None.

ti-chi-bot · 2023-11-02T09:21:49Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-11-02T09:21:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-11-02T09:32:44Z

Codecov Report

Merging #7304 (3eff39e) into master (e6e35fd) will decrease coverage by 0.37%.
The diff coverage is 33.33%.

❗ Current head 3eff39e differs from pull request most recent head 8d19452. Consider uploading reports for the commit 8d19452 to get more accurate results

@@            Coverage Diff             @@
##           master    #7304      +/-   ##
==========================================
- Coverage   74.55%   74.19%   -0.37%     
==========================================
  Files         447      450       +3     
  Lines       48666    48908     +242     
==========================================
+ Hits        36282    36286       +4     
- Misses       9189     9382     +193     
- Partials     3195     3240      +45

Flag	Coverage Δ
unittests	`74.19% <33.33%> (-0.37%)`	⬇️

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

client/http/types.go

nolouch

rest lgtm

nolouch · 2023-11-13T02:57:13Z

client/http/client.go

+}
+
+func (c *client) pdAddr() string {
+	// TODO: support the customized PD address selection strategy.


I think we need it before using it in tidb.

I implemented it to make it the same as https://github.com/pingcap/tidb/blob/6e8df186f51b90d57e29875b03afe317298bc949/pkg/store/helper/helper.go#L900-L906.

nolouch · 2023-11-13T03:03:37Z

client/http/client.go

+	var region RegionInfo
+	err := c.request(ctx, "GetRegionByID", RegionByID(regionID), &region)


TiDB has a metric like tidb_server_pd_api_execution_duration. before using this sdk, It should also be considered to pass the metric into the client for compatibility.

Local test with replaced PD HTTP client:

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

CabinfeverB · 2023-11-13T03:13:13Z

client/http/types.go

+}
+
+// Merge merges two RegionsInfo together and returns a new one.
+func (ri *RegionsInfo) Merge(other *RegionsInfo) *RegionsInfo {


Where it is used?

It will be used here:

pkg/executor/infoschema_reader.go:1609:37: allRegionsInfo.Merge undefined (type *"github.com/tikv/pd/client/http".RegionsInfo has no field or method Merge)

https://github.com/pingcap/tidb/blob/6e8df186f51b90d57e29875b03afe317298bc949/pkg/executor/infoschema_reader.go#L1609

It's better to add a unit test for this function

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

JmPotato · 2023-11-13T05:31:04Z

/merge

ti-chi-bot · 2023-11-13T05:31:07Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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.

ti-chi-bot · 2023-11-13T05:31:08Z

This pull request has been accepted and is ready to merge.

Commit hash: d3f308a

ti-chi-bot · 2023-11-13T05:31:21Z

@JmPotato: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

JmPotato added component/api HTTP API. component/client Client logic. labels Nov 2, 2023

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 2, 2023

ti-chi-bot bot requested review from lhy1024 and nolouch November 2, 2023 09:21

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2023

JmPotato force-pushed the introduce_http_client branch from dbc89cb to 699cbfb Compare November 2, 2023 09:35

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2023

JmPotato force-pushed the introduce_http_client branch 2 times, most recently from 3abf334 to 5d2c497 Compare November 3, 2023 05:52

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 3, 2023

This was referenced Nov 3, 2023

Support to integrate with the PD HTTP client tikv/client-go#1049

Merged

*: integrate PD HTTP client to the store helper pingcap/tidb#48276

Merged

JmPotato force-pushed the introduce_http_client branch from 5d2c497 to acc936a Compare November 6, 2023 08:30

JmPotato marked this pull request as ready for review November 6, 2023 08:30

JmPotato requested review from CabinfeverB, rleungx and HuSharp November 7, 2023 07:26

JmPotato mentioned this pull request Nov 7, 2023

Tracking issue for the HTTP SDK #7300

Open

7 tasks

rleungx reviewed Nov 8, 2023

View reviewed changes

client/http/types.go Show resolved Hide resolved

HuSharp mentioned this pull request Nov 8, 2023

tests: add integration test and ci workflow for real tiup #7317

Closed

rleungx approved these changes Nov 13, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 13, 2023

nolouch reviewed Nov 13, 2023

View reviewed changes

JmPotato added 4 commits November 13, 2023 11:12

Introduce the HTTP client

9548074

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

Refine the namings

76bf32b

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

Add the test suite

af97fda

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

Refine GetMinResolvedTSByStoresIDs and its test

0dbfe3a

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

CabinfeverB reviewed Nov 13, 2023

View reviewed changes

JmPotato force-pushed the introduce_http_client branch from 216b5b7 to f27541c Compare November 13, 2023 03:47

Implement the retry and metrics integration

3eff39e

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

JmPotato force-pushed the introduce_http_client branch from f27541c to 3eff39e Compare November 13, 2023 03:48

JmPotato requested review from nolouch and CabinfeverB November 13, 2023 03:57

JmPotato force-pushed the introduce_http_client branch from 724daee to 3eff39e Compare November 13, 2023 04:00

CabinfeverB approved these changes Nov 13, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 13, 2023

Add TestMergeRegionsInfo

d3f308a

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

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 13, 2023

Merge branch 'master' into introduce_http_client

8d19452

ti-chi-bot bot merged commit 7dbe607 into tikv:master Nov 13, 2023
22 checks passed

JmPotato deleted the introduce_http_client branch November 13, 2023 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: introduce the HTTP client #7304

client: introduce the HTTP client #7304

JmPotato commented Nov 2, 2023

ti-chi-bot bot commented Nov 2, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

nolouch left a comment

nolouch Nov 13, 2023

JmPotato Nov 13, 2023

nolouch Nov 13, 2023

JmPotato Nov 13, 2023

JmPotato Nov 13, 2023

CabinfeverB Nov 13, 2023

JmPotato Nov 13, 2023

JmPotato Nov 13, 2023

CabinfeverB Nov 13, 2023

JmPotato commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

		var region RegionInfo
		err := c.request(ctx, "GetRegionByID", RegionByID(regionID), &region)

client: introduce the HTTP client #7304

client: introduce the HTTP client #7304

Conversation

JmPotato commented Nov 2, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 2, 2023 • edited Loading

ti-chi-bot bot commented Nov 2, 2023

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JmPotato commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

ti-chi-bot bot commented Nov 13, 2023

ti-chi-bot bot commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading