client: use ServiceDiscovery to update member in HTTP client #7668

CabinfeverB · 2024-01-04T10:28:12Z

What problem does this PR solve?

Issue Number: ref #7576

What is changed and how does it work?

use ServiceDiscovery to update member in HTTP client

Check List

Tests

Unit test
Integration test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-01-04T10:28:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
JmPotato

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.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-01-05T08:33:34Z

PR needs rebase.

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.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2024-01-08T07:18:02Z

Codecov Report

Merging #7668 (9d21255) into master (2a2a615) will increase coverage by 0.06%.
The diff coverage is 94.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7668      +/-   ##
==========================================
+ Coverage   73.80%   73.87%   +0.06%     
==========================================
  Files         429      429              
  Lines       47439    47413      -26     
==========================================
+ Hits        35014    35025      +11     
+ Misses       9424     9409      -15     
+ Partials     3001     2979      -22

Flag	Coverage Δ
unittests	`73.87% <94.59%> (+0.06%)`	⬆️

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

JmPotato · 2024-01-08T16:05:26Z

client/http/client.go

@@ -58,8 +57,8 @@ type clientInner struct {
 	cancel context.CancelFunc

 	sync.RWMutex


Is this lock still needed?

JmPotato · 2024-01-08T16:08:19Z

client/http/client.go

+	sd pd.ServiceDiscovery,
+	opts ...ClientOption,
+) Client {
+	ctx, cancel := context.WithCancel(context.Background())


Should use a context passed through.

I found out that when we created innerClient, it created a context itself and cancel it with the Close function, so I kept it. Do we need to change it?

Forgot about the logic. Then let's keep it the same.

JmPotato · 2024-01-08T16:11:56Z

client/http/client.go

-	if leaderAddrIdx != -1 {
-		addr = pdAddrs[leaderAddrIdx]
+	clients := ci.sd.GetAllServiceClients()
+	for _, cli := range clients {


Can we maintain the original logic of requesting the leader first and then the followers? This is to reduce the unnecessary redirect as much as possible.

Yes, the slice returned by GetAllServiceClients will hold the leader as the first place. Let me add some comments.

JmPotato · 2024-01-08T16:14:21Z

client/pd_service_discovery.go

-func newPDServiceClient(addr, leaderAddr string, conn *grpc.ClientConn, isLeader bool) ServiceClient {
+func newPDServiceClient(addr, leaderAddr string, tlsCfg *tls.Config, conn *grpc.ClientConn, isLeader bool) ServiceClient {
+	var httpAddress string
+	if strings.HasPrefix(addr, httpScheme) {


What if the address starts with http but still has a TLS config passed in? In this case, replacing the scheme is also needed.

Good catch. It seems that we didn't handle this case well before.

JmPotato · 2024-01-08T16:15:35Z

client/pd_service_discovery.go

@@ -732,6 +780,21 @@ func (c *pdServiceDiscovery) GetServiceClient() ServiceClient {
 	return leaderClient
 }

+// GetAllServiceClients implments ServiceDiscovery
+func (c *pdServiceDiscovery) GetAllServiceClients() []ServiceClient {


Maybe return leader and follower addresses respectively could help with the above comment.

It's both OK to me. My idea is that we only need one for loop, so I just return all clients together

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2024-01-09T05:31:43Z

client/http/client.go

 	// Apply the options first.
 	for _, opt := range opts {
 		opt(c)
 	}
-	c.inner.setPDAddrs(pdAddrs, -1)
-	c.inner.init()
+	sd := pd.NewDefaultPDServiceDiscovery(ctx, cancel, pdAddrs, c.inner.tlsConf)


why not just use newPDServiceDiscovery?

OK. I didn't think it was a good idea to pass in a lot of nil params before.

I forgot that option is unexported, so I think we can use NewDefaultPDServiceDiscovery to avoid to input some unconcerned params.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2024-01-09T11:10:02Z

client/http/client_test.go

@@ -101,77 +65,6 @@ func TestCallerID(t *testing.T) {
 	c.Close()
 }

-func TestRedirectWithMetrics(t *testing.T) {


move it into integration test.

JmPotato · 2024-01-09T15:29:58Z

client/http/client.go

+	sd pd.ServiceDiscovery,
+	opts ...ClientOption,
+) Client {
+	ctx, cancel := context.WithCancel(context.Background())


Forgot about the logic. Then let's keep it the same.

JmPotato · 2024-01-09T15:31:27Z

client/http/client.go

+type requestChecker struct {
+	checker func(req *http.Request) error
 }


What about just:

Suggested change

type requestChecker struct {

checker func(req *http.Request) error

}

type requestChecker func(req *http.Request) error

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2024-01-10T01:33:53Z

client/http/client.go

-func (c *client) UpdateMembersInfo() {
-	c.inner.updateMembersInfo(c.inner.ctx)
+// requestChecker is used to check the HTTP request sent by the client.
+type requestChecker func(req *http.Request) error


cc @HuSharp

Is it just used in the test?

maybe better to add some comments for NewHTTPClientWithRequestChecker

client/retry/backoff.go

client/http/client.go

HuSharp · 2024-01-10T01:48:42Z

client/pd_service_discovery.go

@@ -732,6 +792,21 @@ func (c *pdServiceDiscovery) GetServiceClient() ServiceClient {
 	return leaderClient
 }

+// GetAllServiceClients implments ServiceDiscovery
+func (c *pdServiceDiscovery) GetAllServiceClients() []ServiceClient {
+	ret := make([]ServiceClient, 0)


Is it better to specify make(client, 1) or another larger value? perhaps the number of clients in the main scenario is greater than 0.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

client/http/client.go

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2024-01-12T06:18:18Z

/merge

ti-chi-bot · 2024-01-12T06:18:20Z

@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 · 2024-01-12T06:18:22Z

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

Commit hash: 9d21255

ref tikv#7576 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Signed-off-by: pingandb <songge102@pingan.com.cn>

CabinfeverB added 4 commits January 4, 2024 15:30

directly save tls config

3917a61

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

directly save tls config

f09af97

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

120728c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

use ServiceDiscovery for http client

5ba9e63

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB requested review from JmPotato, rleungx and HuSharp January 4, 2024 10:28

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 Jan 4, 2024

use ServiceDiscovery for http client

0606fbf

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

CabinfeverB added 2 commits January 5, 2024 16:52

use ServiceDiscovery for http client

12d600c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

449bb8f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB mentioned this pull request Jan 8, 2024

client/http: add scheduleUpdateMembersInfo to get the latest members ASAP #7675

Closed

merge master

f392d75

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato reviewed Jan 8, 2024

View reviewed changes

address comment

2b7b72e

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2024

rleungx reviewed Jan 9, 2024

View reviewed changes

merge master

faa9261

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB commented Jan 9, 2024

View reviewed changes

JmPotato reviewed Jan 9, 2024

View reviewed changes

CabinfeverB added 2 commits January 10, 2024 09:26

fix test

13945a6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

3ddabcb

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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 Jan 10, 2024

CabinfeverB commented Jan 10, 2024

View reviewed changes

HuSharp reviewed Jan 10, 2024

View reviewed changes

client/retry/backoff.go Outdated Show resolved Hide resolved

HuSharp reviewed Jan 10, 2024

View reviewed changes

client/http/client.go Outdated Show resolved Hide resolved

HuSharp reviewed Jan 10, 2024

View reviewed changes

address comment

a26d1c6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

HuSharp reviewed Jan 10, 2024

View reviewed changes

client/http/client.go Outdated Show resolved Hide resolved

address comment

8b65ba7

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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 Jan 10, 2024

CabinfeverB added 2 commits January 10, 2024 10:28

address comment

3757435

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

9d21255

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato approved these changes Jan 12, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 12, 2024

HuSharp approved these changes Jan 12, 2024

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 Jan 12, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2024

ti-chi-bot bot merged commit 9cdbeb6 into tikv:master Jan 12, 2024
25 of 26 checks passed

pingandb pushed a commit to pingandb/pd that referenced this pull request Jan 18, 2024

client: use ServiceDiscovery to update member in HTTP client (tikv#7668)

b32fd5a

ref tikv#7576 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Signed-off-by: pingandb <songge102@pingan.com.cn>

HuSharp mentioned this pull request Jan 19, 2024

pctl: release-7.6 ctl failed to get stores using tls #7739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: use ServiceDiscovery to update member in HTTP client #7668

client: use ServiceDiscovery to update member in HTTP client #7668

CabinfeverB commented Jan 4, 2024

ti-chi-bot bot commented Jan 4, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 5, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

JmPotato Jan 8, 2024

JmPotato Jan 8, 2024

CabinfeverB Jan 9, 2024

JmPotato Jan 9, 2024

JmPotato Jan 8, 2024

CabinfeverB Jan 9, 2024

JmPotato Jan 8, 2024

CabinfeverB Jan 9, 2024

JmPotato Jan 8, 2024

CabinfeverB Jan 9, 2024

rleungx Jan 9, 2024

CabinfeverB Jan 9, 2024

CabinfeverB Jan 9, 2024

CabinfeverB Jan 9, 2024

JmPotato Jan 9, 2024

JmPotato Jan 9, 2024

CabinfeverB Jan 10, 2024

HuSharp Jan 10, 2024

CabinfeverB Jan 10, 2024

HuSharp Jan 10, 2024

HuSharp Jan 10, 2024

JmPotato commented Jan 12, 2024

ti-chi-bot bot commented Jan 12, 2024

ti-chi-bot bot commented Jan 12, 2024

		@@ -58,8 +57,8 @@ type clientInner struct {
		cancel context.CancelFunc

		sync.RWMutex

client: use ServiceDiscovery to update member in HTTP client #7668

client: use ServiceDiscovery to update member in HTTP client #7668

Conversation

CabinfeverB commented Jan 4, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jan 4, 2024 • edited Loading

ti-chi-bot bot commented Jan 5, 2024

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

JmPotato commented Jan 12, 2024

ti-chi-bot bot commented Jan 12, 2024

ti-chi-bot bot commented Jan 12, 2024

ti-chi-bot bot commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading