config: refactor TSO config #5902

rleungx · 2023-02-02T10:10:55Z

What problem does this PR solve?

Issue Number: Ref #5839.

What is changed and how does it work?

This PR can be reviewed after #5901 is merged.

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2023-02-02T10:10:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lhy1024
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.

rleungx · 2023-02-02T10:11:32Z

@binshi-bing PTAL

codecov · 2023-02-02T10:21:38Z

Codecov Report

Base: 75.56% // Head: 75.47% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (295a60f) compared to base (393b287).
Patch coverage: 84.37% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5902      +/-   ##
==========================================
- Coverage   75.56%   75.47%   -0.09%     
==========================================
  Files         344      345       +1     
  Lines       35121    35126       +5     
==========================================
- Hits        26538    26512      -26     
- Misses       6303     6320      +17     
- Partials     2280     2294      +14

Flag	Coverage Δ
unittests	`75.47% <84.37%> (-0.09%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/tso/config.go	`0.00% <0.00%> (ø)`
pkg/member/member.go	`62.10% <100.00%> (-3.69%)`	⬇️
pkg/tso/allocator_manager.go	`62.25% <100.00%> (-0.81%)`	⬇️
server/config/config.go	`78.71% <100.00%> (ø)`
server/server.go	`74.90% <100.00%> (-0.64%)`	⬇️
pkg/errs/errs.go	`75.00% <0.00%> (-25.00%)`	⬇️
pkg/tso/local_allocator.go	`64.86% <0.00%> (-13.52%)`	⬇️
pkg/storage/endpoint/rule.go	`83.33% <0.00%> (-11.12%)`	⬇️
pkg/utils/tempurl/tempurl.go	`60.00% <0.00%> (-10.00%)`	⬇️
pkg/election/leadership.go	`75.25% <0.00%> (-5.16%)`	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Ryan Leung <[email protected]>

binshi-bing

LGTM. Thanks for making this change!

ti-chi-bot · 2023-02-03T02:52:45Z

@binshi-bing: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM. Thanks for making this change!

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.

pkg/tso/allocator_manager.go

ti-chi-bot · 2023-02-05T20:22:58Z

@binshi-bing: Request changes is only allowed for the reviewers in list.

In response to this:

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.

server/config/config.go

Signed-off-by: Ryan Leung <[email protected]>

binshi-bing · 2023-02-06T04:46:08Z

pkg/tso/allocator_manager.go

@@ -134,17 +132,20 @@ type AllocatorManager struct {
 func NewAllocatorManager(
 	m *member.Member,
 	rootPath string,
-	cfg config,
+	enableLocalTSO bool,


passing all tso configuration fields here seems to be a litter weird, but we can refine later.

Yes, we can define an interface later to avoid it.

binshi-bing

LGTM

Signed-off-by: Ryan Leung <[email protected]>

binshi-bing

LGTM

ti-chi-bot · 2023-02-06T05:21:53Z

@binshi-bing: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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.

rleungx · 2023-02-06T05:22:10Z

server/config/config.go

@@ -613,6 +570,49 @@ func (c *Config) Adjust(meta *toml.MetaData, reloading bool) error {
 	return nil
 }

+// Utility to test if a configuration is defined.
+type configMetaData struct {


This part will be moved to a common package later.

nolouch

lgtm

JmPotato

The rest LGTM,

JmPotato · 2023-02-06T09:26:08Z

server/config/config.go

+// Utility to test if a configuration is defined.
+type configMetaData struct {
+	meta *toml.MetaData
+	path []string


This path is kind of confusing to me. Does it mean the config file path or the internal config key path? What about using keyPath?

configMetaData is an existing structure and the issue is a legacy issue. This PR just moves configMetaData from other place without modifying anything except the place, so I think we can address @JmPotato's concern in separate PR.

lhy1024 · 2023-02-06T19:07:44Z

/merge

ti-chi-bot · 2023-02-06T19:07:46Z

@lhy1024: 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-02-06T19:07:47Z

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

Commit hash: b244f5e

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

rleungx requested a review from lhy1024 February 2, 2023 10:11

ti-chi-bot requested review from disksing and HunDunDM February 2, 2023 10:11

rleungx requested review from JmPotato and removed request for disksing and HunDunDM February 2, 2023 10:11

refactor tso config

ff04454

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the tso-config branch from 0243346 to ff04454 Compare February 3, 2023 01:54

binshi-bing approved these changes Feb 3, 2023

View reviewed changes

rleungx requested a review from nolouch February 3, 2023 06:44

binshi-bing requested changes Feb 5, 2023

View reviewed changes

pkg/tso/allocator_manager.go Outdated Show resolved Hide resolved

binshi-bing reviewed Feb 5, 2023

View reviewed changes

server/config/config.go Outdated Show resolved Hide resolved

binshi-bing reviewed Feb 5, 2023

View reviewed changes

server/config/config.go Outdated Show resolved Hide resolved

binshi-bing reviewed Feb 5, 2023

View reviewed changes

server/config/config.go Outdated Show resolved Hide resolved

address comments

f455d39

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the tso-config branch from 89ab518 to f455d39 Compare February 6, 2023 04:09

rleungx requested a review from binshi-bing February 6, 2023 04:10

binshi-bing reviewed Feb 6, 2023

View reviewed changes

address comments

b244f5e

Signed-off-by: Ryan Leung <[email protected]>

rleungx requested a review from binshi-bing February 6, 2023 05:16

binshi-bing approved these changes Feb 6, 2023

View reviewed changes

rleungx commented Feb 6, 2023

View reviewed changes

lhy1024 approved these changes Feb 6, 2023

View reviewed changes

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

nolouch approved these changes Feb 6, 2023

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 Feb 6, 2023

JmPotato reviewed Feb 6, 2023

View reviewed changes

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

Merge branch 'master' into tso-config

295a60f

ti-chi-bot merged commit 2a7c8d4 into tikv:master Feb 6, 2023

rleungx deleted the tso-config branch February 7, 2023 01:55

binshi-bing mentioned this pull request Feb 7, 2023

Separate TSO into an independent service #5836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: refactor TSO config #5902

config: refactor TSO config #5902

rleungx commented Feb 2, 2023

ti-chi-bot commented Feb 2, 2023 •

edited

Loading

rleungx commented Feb 2, 2023

codecov bot commented Feb 2, 2023 •

edited

Loading

binshi-bing left a comment

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 5, 2023

binshi-bing Feb 6, 2023

rleungx Feb 6, 2023

binshi-bing left a comment

binshi-bing left a comment

ti-chi-bot commented Feb 6, 2023

rleungx Feb 6, 2023

nolouch left a comment

JmPotato left a comment

JmPotato Feb 6, 2023

binshi-bing Feb 6, 2023

lhy1024 commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

config: refactor TSO config #5902

config: refactor TSO config #5902

Conversation

rleungx commented Feb 2, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

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

rleungx commented Feb 2, 2023

codecov bot commented Feb 2, 2023 • edited Loading

Codecov Report

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 3, 2023

ti-chi-bot commented Feb 5, 2023

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

rleungx Feb 6, 2023

Choose a reason for hiding this comment

binshi-bing left a comment

Choose a reason for hiding this comment

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 6, 2023

rleungx Feb 6, 2023

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

JmPotato left a comment

Choose a reason for hiding this comment

JmPotato Feb 6, 2023

Choose a reason for hiding this comment

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

lhy1024 commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

ti-chi-bot commented Feb 2, 2023 •

edited

Loading

codecov bot commented Feb 2, 2023 •

edited

Loading