*: use another etcd client for election #6409

rleungx · 2023-05-05T06:48:55Z

What problem does this PR solve?

Issue Number: Ref #6403.

What is changed and how does it work?

This PR tries to use another etcd client for the election to prevent the throttling of one connection.

Check List

Tests

Manual test (TODO)

Release note

None.

ti-chi-bot · 2023-05-05T06:48:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

binshi-bing
lhy1024

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.

codecov · 2023-05-05T07:05:09Z

Codecov Report

Patch coverage: 50.00% and project coverage change: +0.03 🎉

Comparison is base (1f4599f) 75.03% compared to head (40976cb) 75.07%.

❗ Current head 40976cb differs from pull request most recent head 343bda6. Consider uploading reports for the commit 343bda6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6409      +/-   ##
==========================================
+ Coverage   75.03%   75.07%   +0.03%     
==========================================
  Files         409      408       -1     
  Lines       41532    41113     -419     
==========================================
- Hits        31165    30867     -298     
+ Misses       7659     7560      -99     
+ Partials     2708     2686      -22

Flag	Coverage Δ
unittests	`75.07% <50.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/utils/etcdutil/etcdutil.go	`81.57% <40.00%> (+1.78%)`	⬆️
server/server.go	`74.60% <42.85%> (-0.43%)`	⬇️
pkg/mcs/resource_manager/server/server.go	`52.99% <100.00%> (ø)`
pkg/mcs/tso/server/server.go	`49.16% <100.00%> (-4.29%)`	⬇️

... and 61 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lhy1024 · 2023-05-05T08:02:22Z

pkg/utils/etcdutil/etcdutil.go

 // CreateClients creates etcd v3 client and http client.
-func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *http.Client, error) {
+func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *clientv3.Client, *http.Client, error) {


return two etcdClient? maybe we can directly create electionClient with another function?

+1. The change in pr isn't clean

Just call "electionClient, err := createEtcdClient(tlsConfig, acUrls)" to create a dedicate election etcd client is clearer.

Hmmm... I'm trying to avoid duplicated config initialization.

just to add a createEtcdClient in server.go?

+1, we can create two clients in startClient not here.

bufferflies · 2023-05-08T11:01:22Z

server/server.go

 	return nil
 }

-func startClient(cfg *config.Config) (*clientv3.Client, *http.Client, error) {
+func startClient(cfg *config.Config) (*clientv3.Client, *clientv3.Client, *http.Client, error) {


why not use array? Maybe putting them into on struct is better?

binshi-bing · 2023-05-08T23:32:12Z

pkg/utils/etcdutil/etcdutil.go

 // CreateClients creates etcd v3 client and http client.
-func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *http.Client, error) {
+func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *clientv3.Client, *http.Client, error) {


+1. The change in pr isn't clean

binshi-bing · 2023-05-08T23:33:01Z

pkg/utils/etcdutil/etcdutil.go

 // CreateClients creates etcd v3 client and http client.
-func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *http.Client, error) {
+func CreateClients(tlsConfig *tls.Config, acUrls url.URL) (*clientv3.Client, *clientv3.Client, *http.Client, error) {


Just call "electionClient, err := createEtcdClient(tlsConfig, acUrls)" to create a dedicate election etcd client is clearer.

Signed-off-by: Ryan Leung <rleungx@gmail.com>

nolouch

lgtm

nolouch · 2023-05-12T08:24:43Z

/merge

ti-chi-bot · 2023-05-12T08:24:44Z

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

/run-all-tests

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-05-12T08:24:46Z

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

Commit hash: 343bda6

ti-chi-bot · 2023-05-12T08:45:03Z

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

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

ti-chi-bot · 2023-05-12T08:45:41Z

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

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

ref #6403, ref #6409 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 5, 2023

ti-chi-bot bot requested review from disksing and lhy1024 May 5, 2023 06:49

rleungx force-pushed the election-client branch from ed5aadf to 40976cb Compare May 5, 2023 07:08

ti-chi-bot bot added the do-not-merge/needs-triage-completed label May 5, 2023

lhy1024 reviewed May 5, 2023

View reviewed changes

rleungx requested review from nolouch and removed request for disksing May 6, 2023 02:34

bufferflies reviewed May 8, 2023

View reviewed changes

binshi-bing approved these changes May 8, 2023

View reviewed changes

lhy1024 approved these changes May 12, 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 May 12, 2023

use another etcd client for election

e1af99b

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the election-client branch from 790671d to 92266be Compare May 12, 2023 07:11

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

rleungx force-pushed the election-client branch from 92266be to 74605f1 Compare May 12, 2023 07:12

address comments

343bda6

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the election-client branch from 74605f1 to 343bda6 Compare May 12, 2023 07:21

nolouch approved these changes May 12, 2023

View reviewed changes

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

ti-chi-bot bot merged commit d2e73d1 into tikv:master May 12, 2023

ti-chi-bot mentioned this pull request May 12, 2023

*: use another etcd client for election (#6409) #6454

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request May 12, 2023

This is an automated cherry-pick of tikv#6409

aa26d8e

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

ti-chi-bot mentioned this pull request May 12, 2023

*: use another etcd client for election (#6409) #6455

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request May 12, 2023

This is an automated cherry-pick of tikv#6409

fab219f

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

rleungx deleted the election-client branch May 17, 2023 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: use another etcd client for election #6409

*: use another etcd client for election #6409

rleungx commented May 5, 2023

ti-chi-bot bot commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

lhy1024 May 5, 2023

binshi-bing May 8, 2023

binshi-bing May 8, 2023

rleungx May 9, 2023

lhy1024 May 12, 2023

nolouch May 12, 2023

bufferflies May 8, 2023 •

edited

Loading

binshi-bing May 8, 2023

binshi-bing May 8, 2023

nolouch left a comment

nolouch commented May 12, 2023

ti-chi-bot bot commented May 12, 2023

ti-chi-bot bot commented May 12, 2023

ti-chi-bot commented May 12, 2023

ti-chi-bot commented May 12, 2023

*: use another etcd client for election #6409

*: use another etcd client for election #6409

Conversation

rleungx commented May 5, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 5, 2023 • edited Loading

codecov bot commented May 5, 2023 • 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

bufferflies May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented May 12, 2023

ti-chi-bot bot commented May 12, 2023

ti-chi-bot bot commented May 12, 2023

ti-chi-bot commented May 12, 2023

ti-chi-bot commented May 12, 2023

ti-chi-bot bot commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

bufferflies May 8, 2023 •

edited

Loading