Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tso: implement the Global TSO optimization #3390

Merged
merged 32 commits into from
May 27, 2021
Merged

tso: implement the Global TSO optimization #3390

merged 32 commits into from
May 27, 2021

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Jan 26, 2021 •
edited
Loading

Signed-off-by: JmPotato [email protected]

What problem does this PR solve?

Part of #2759. Resolve #3345. Also PTAL at pingcap/kvproto#716.

What is changed and how it works?

Check List

Tests

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

Release note

@JmPotato JmPotato added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/tso Timestamp Oracle. labels Jan 26, 2021
@JmPotato JmPotato removed the request for review from HunDunDM January 26, 2021 07:59
@ti-chi-bot 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 Jan 28, 2021
@JmPotato JmPotato force-pushed the refine_global_tso branch 3 times, most recently from b77f5da to 43a7d02 Compare January 29, 2021 08:43
@ti-chi-bot 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 Feb 4, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2021
@JmPotato JmPotato force-pushed the refine_global_tso branch 2 times, most recently from ef92962 to 8400f56 Compare February 24, 2021 11:05
@ti-chi-bot 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 Feb 25, 2021
@JmPotato JmPotato closed this Mar 2, 2021
@JmPotato JmPotato reopened this Mar 2, 2021
@JmPotato JmPotato force-pushed the refine_global_tso branch 2 times, most recently from 7e90951 to 92c765a Compare March 8, 2021 04:38
@JmPotato JmPotato marked this pull request as ready for review March 8, 2021 06:24
@ti-chi-bot 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 Mar 8, 2021
@ti-chi-bot 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 Mar 14, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2021
rleungx
rleungx reviewed May 18, 2021
View reviewed changes
server/grpc_service.go Outdated Show resolved Hide resolved
server/grpc_service.go Outdated Show resolved Hide resolved
@JmPotato
Address the comment
e426017 
Signed-off-by: JmPotato <[email protected]>
rleungx
rleungx reviewed May 19, 2021
View reviewed changes
Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@JmPotato
Resolve the conflict
6545f32 
Signed-off-by: JmPotato <[email protected]>
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@sonarcloud
Copy link

sonarcloud bot commented May 19, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@JmPotato
Copy link
Member Author

/run-all-tests

nolouch
nolouch reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest lgtm

server/tso/global_allocator.go
// we need to validate it first before we write it into every Local TSO Allocator's memory.
globalTSOResp = *estimatedMaxTSO
if err = gta.SyncMaxTS(ctx, dcLocationMap, &globalTSOResp, skipCheck); err != nil {
return pdpb.Timestamp{}, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need metrics about the failed counter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary, and I will add some metrics in next PR.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Yisaer
  • 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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot 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 May 25, 2021
@nolouch
Copy link
Contributor

nolouch commented May 25, 2021

need update the proto.

@JmPotato
Update kvproto
19b53fd 
Signed-off-by: JmPotato <[email protected]>
@JmPotato
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

@JmPotato: 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
Copy link
Member

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

Commit hash: 19b53fd

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 27, 2021
@ti-chi-bot ti-chi-bot merged commit b157862 into tikv:master May 27, 2021
@JmPotato JmPotato deleted the refine_global_tso branch May 27, 2021 09:38
@JmPotato JmPotato mentioned this pull request May 28, 2021
Closed
lhy1024 pushed a commit to lhy1024/pd that referenced this pull request Jun 2, 2021
@JmPotato @ti-chi-bot @lhy1024
tso: implement the Global TSO optimization (tikv#3390)
7e84d1a 
* Implement the Global TSO optimization

Signed-off-by: JmPotato <[email protected]>

* Merge the upstream codes

Signed-off-by: JmPotato <[email protected]>

* Add more comments

Signed-off-by: JmPotato <[email protected]>

* Add back the missing updateTime update statement

Signed-off-by: JmPotato <[email protected]>

* Refine codes and comments

Signed-off-by: JmPotato <[email protected]>

* Update kvproto version

Signed-off-by: JmPotato <[email protected]>

* Refine some comments

Signed-off-by: JmPotato <[email protected]>

* Add a switch for Global TSO estimation

Signed-off-by: JmPotato <[email protected]>

* Address comments

Signed-off-by: JmPotato <[email protected]>

* Fix the problem of physical time advancing too fast

Signed-off-by: JmPotato <[email protected]>

* Fix the bugs of Local TSO physical time advancing too fast and TSO fallback

Signed-off-by: JmPotato <[email protected]>

* Reduce unnecessary memory usage

Signed-off-by: JmPotato <[email protected]>

* Reduce unnecessary memory usage

Signed-off-by: JmPotato <[email protected]>

* Fix the bug that tsoObject may be reset to a smaller one

Signed-off-by: JmPotato <[email protected]>

* Fix the static check

Signed-off-by: JmPotato <[email protected]>

* Fix typos

Signed-off-by: JmPotato <[email protected]>

* Address the comment

Signed-off-by: JmPotato <[email protected]>

* Remove enableGlobalTSOEstimation switch

Signed-off-by: JmPotato <[email protected]>

* Update kvproto

Signed-off-by: JmPotato <[email protected]>

Co-authored-by: Ti Chi Robot <[email protected]>
Signed-off-by: lhy1024 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@nolouch nolouch nolouch approved these changes

@Yisaer Yisaer Yisaer approved these changes

@HunDunDM HunDunDM Awaiting requested review from HunDunDM

Assignees
No one assigned
Labels
component/tso Timestamp Oracle. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimize the latency performance of Global TSO generation
5 participants
@JmPotato @ti-chi-bot @nolouch @Yisaer @rleungx