client: introduce UpdateOption method to configure the PD client online #4240

JmPotato · 2021-10-26T07:46:16Z

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

What problem does this PR solve?

Part of #3149. Should be merged after #4204.

What is changed and how it works?

Introduce ClientOption to configure the PD client in a more flexible way.

Check List

Tests

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

Side effects

Increased code complexity

Related changes

PR client: make the server selection for TSO proxy requests more random #4204

Release note

None.

ti-chi-bot · 2021-10-26T07:46:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

disksing
nolouch

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 · 2021-10-26T12:27:15Z

Codecov Report

Merging #4240 (ab33695) into master (400344b) will decrease coverage by 0.02%.
The diff coverage is 73.62%.

@@            Coverage Diff             @@
##           master    #4240      +/-   ##
==========================================
- Coverage   74.81%   74.78%   -0.03%     
==========================================
  Files         260      261       +1     
  Lines       26827    26871      +44     
==========================================
+ Hits        20070    20095      +25     
- Misses       4974     4986      +12     
- Partials     1783     1790       +7

Flag	Coverage Δ
unittests	`74.78% <73.62%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
client/client.go	`71.52% <68.25%> (-0.69%)`	⬇️
client/base_client.go	`79.89% <75.00%> (-2.25%)`	⬇️
client/option.go	`87.50% <87.50%> (ø)`
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
server/schedulers/shuffle_hot_region.go	`55.55% <0.00%> (-10.11%)`	⬇️
server/schedule/region_scatterer.go	`80.70% <0.00%> (-4.39%)`	⬇️
server/server.go	`71.85% <0.00%> (-0.90%)`	⬇️
server/schedule/operator_controller.go	`82.68% <0.00%> (-0.82%)`	⬇️
server/schedule/operator/step.go	`72.32% <0.00%> (-0.74%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 400344b...ab33695. Read the comment docs.

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

nolouch

rest LGTM

client/client.go

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

disksing · 2021-10-29T07:16:10Z

tests/client/client_test.go

+	cli.UpdateOption(pd.EnableTSOFollowerProxy, false)
+	// There will be a stream has been chosen before when the TSO Follower Proxy is enabled.
+	// We need to consume it before the client starts the next round of TSO request batch.
+	// TODO: fix this corner case.


Better to create an issue for it.

OK. I will create one after this PR is merged.

nolouch · 2021-10-29T08:34:04Z

/merge

ti-chi-bot · 2021-10-29T08:34:05Z

@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 · 2021-10-29T08:34:07Z

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

Commit hash: ab33695

…ne (tikv#4240) * Introduce ClientOption to configure the PD client (ref tikv#3149) Signed-off-by: JmPotato <ghzpotato@gmail.com> * Refine the code and add some comments Signed-off-by: JmPotato <ghzpotato@gmail.com> * Clean the code Signed-off-by: JmPotato <ghzpotato@gmail.com> * Use a interface method to update the client option Signed-off-by: JmPotato <ghzpotato@gmail.com> * Update the newOption Signed-off-by: JmPotato <ghzpotato@gmail.com> * Remove WithTSOFollowerProxy and WithMaxTSOBatchWaitInterval Signed-off-by: JmPotato <ghzpotato@gmail.com> * Address the comments and refine the test Signed-off-by: JmPotato <ghzpotato@gmail.com> Signed-off-by: hzh0425 <642256541@qq.com>

JmPotato added the component/client Client logic. label Oct 26, 2021

JmPotato requested review from nolouch, HunDunDM and rleungx October 26, 2021 07:46

ti-chi-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 Oct 26, 2021

JmPotato force-pushed the tso_follower_proxy_switch branch 2 times, most recently from 7cb285f to 3c9c7b5 Compare October 26, 2021 07:53

JmPotato mentioned this pull request Oct 26, 2021

Update the PD client to the latest version tikv/client-go#352

Merged

JmPotato marked this pull request as ready for review October 26, 2021 11:19

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2021

JmPotato mentioned this pull request Oct 26, 2021

*: add two session variables to control the PD client option pingcap/tidb#29120

Merged

3 tasks

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 26, 2021

JmPotato force-pushed the tso_follower_proxy_switch branch 2 times, most recently from 3a6c1b7 to 5bc98a6 Compare October 28, 2021 13:56

Introduce ClientOption to configure the PD client (ref tikv#3149)

216c0fa

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

JmPotato force-pushed the tso_follower_proxy_switch branch from 5bc98a6 to 216c0fa Compare October 28, 2021 14:09

JmPotato requested a review from disksing October 28, 2021 16:32

JmPotato added 3 commits October 29, 2021 01:16

Refine the code and add some comments

24f73fb

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

Clean the code

e2d90b5

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

Use a interface method to update the client option

20ed47c

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

JmPotato changed the title ~~client: introduce ClientOption to configure the PD client~~ client: introduce UpdateOption method to configure the PD client online Oct 29, 2021

JmPotato added 2 commits October 29, 2021 12:16

Update the newOption

925341f

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

Remove WithTSOFollowerProxy and WithMaxTSOBatchWaitInterval

739e252

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

JmPotato force-pushed the tso_follower_proxy_switch branch from 3f20998 to 739e252 Compare October 29, 2021 04:53

nolouch reviewed Oct 29, 2021

View reviewed changes

client/client.go Outdated Show resolved Hide resolved

client/client.go Outdated Show resolved Hide resolved

Address the comments and refine the test

ab33695

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

disksing approved these changes Oct 29, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 29, 2021

nolouch approved these changes Oct 29, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 29, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 29, 2021

ti-chi-bot merged commit e65f0c5 into tikv:master Oct 29, 2021

JmPotato deleted the tso_follower_proxy_switch branch October 29, 2021 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: introduce UpdateOption method to configure the PD client online #4240

client: introduce UpdateOption method to configure the PD client online #4240

JmPotato commented Oct 26, 2021

ti-chi-bot commented Oct 26, 2021 •

edited

Loading

codecov bot commented Oct 26, 2021 •

edited

Loading

nolouch left a comment

disksing Oct 29, 2021

JmPotato Oct 29, 2021

JmPotato Oct 29, 2021

nolouch commented Oct 29, 2021

ti-chi-bot commented Oct 29, 2021

ti-chi-bot commented Oct 29, 2021

client: introduce UpdateOption method to configure the PD client online #4240

client: introduce UpdateOption method to configure the PD client online #4240

Conversation

JmPotato commented Oct 26, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 26, 2021 • edited Loading

codecov bot commented Oct 26, 2021 • edited Loading

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

disksing Oct 29, 2021

Choose a reason for hiding this comment

JmPotato Oct 29, 2021

Choose a reason for hiding this comment

JmPotato Oct 29, 2021

Choose a reason for hiding this comment

nolouch commented Oct 29, 2021

ti-chi-bot commented Oct 29, 2021

ti-chi-bot commented Oct 29, 2021

ti-chi-bot commented Oct 26, 2021 •

edited

Loading

codecov bot commented Oct 26, 2021 •

edited

Loading