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: add basic election logic for Local TSO Allocator #2894

Merged
merged 21 commits into from
Sep 9, 2020
Merged

tso: add basic election logic for Local TSO Allocator #2894

merged 21 commits into from
Sep 9, 2020

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Sep 3, 2020 •
edited
Loading

Signed-off-by: JmPotato [email protected]

What problem does this PR solve?

Part of #2759. Add basic election logic for Local TSO Allocator.

What is changed and how it works?

Use a similar logic with leaderLoop in server/server.go.

Check List

Tests

  • Unit test

Release note

  • Add basic election logic for Local TSO Allocator.

@JmPotato JmPotato added the component/tso Timestamp Oracle. label Sep 3, 2020
Yisaer
Yisaer reviewed Sep 4, 2020
View reviewed changes
server/tso/local_allocator.go Show resolved Hide resolved
server/tso/local_allocator.go Outdated Show resolved Hide resolved
server/tso/allocator_manager.go Outdated Show resolved Hide resolved
server/tso/allocator_manager.go Show resolved Hide resolved
@JmPotato
Address comments
3bbf035 
Signed-off-by: JmPotato <[email protected]>
@JmPotato JmPotato mentioned this pull request Sep 4, 2020
Closed
16 tasks
@JmPotato
Typo fix
fb50734 
Signed-off-by: JmPotato <[email protected]>
@rleungx
Copy link
Member

rleungx commented Sep 4, 2020

Would you like to add more tests about how mixed TSO allocators work?

@JmPotato
Copy link
Member Author

JmPotato commented Sep 4, 2020

Would you like to add more tests about how mixed TSO allocators work?

It's on the plan. I thought this pr is huge enough to review, so I didn't take the tests with it. As you request, maybe I should add them?

Yisaer
Yisaer reviewed Sep 7, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer 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

server/tso/allocator_manager.go Outdated Show resolved Hide resolved
server/tso/allocator_manager.go Outdated Show resolved Hide resolved
@JmPotato
Address comments
2a34f0c 
Signed-off-by: JmPotato <[email protected]>
@JmPotato JmPotato requested a review from Yisaer September 7, 2020 04:48
Yisaer
Yisaer approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 7, 2020
@JmPotato
Copy link
Member Author

JmPotato commented Sep 7, 2020

One more review needed. PTAL @rleungx @disksing @lhy1024 @nolouch

@JmPotato
Update allocatorGroup filter
24754ea 
Signed-off-by: JmPotato <[email protected]>
@JmPotato
Bug fix
f0cdbc8 
Signed-off-by: JmPotato <[email protected]>
nolouch
nolouch reviewed Sep 8, 2020
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

tests/server/tso/allocator_test.go Outdated Show resolved Hide resolved
nolouch
nolouch reviewed Sep 8, 2020
View reviewed changes
server/tso/allocator_manager.go Show resolved Hide resolved
tests/server/tso/allocator_test.go Outdated Show resolved Hide resolved
tests/server/tso/allocator_test.go Outdated Show resolved Hide resolved
@JmPotato
Typo and bug fix
14eecbc 
Signed-off-by: JmPotato <[email protected]>
@JmPotato
Copy link
Member Author

JmPotato commented Sep 8, 2020

/run-all-tests

nolouch
nolouch reviewed Sep 8, 2020
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.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 8, 2020
@lhy1024
Copy link
Contributor

lhy1024 commented Sep 8, 2020

/run-integration-lightning-test

@HunDunDM
Copy link
Member

HunDunDM commented Sep 9, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 9, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 58d478c into tikv:master Sep 9, 2020
@JmPotato JmPotato deleted the elect_local_tso_allocator branch September 9, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nolouch nolouch nolouch left review comments

@Yisaer Yisaer Yisaer approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@disksing disksing Awaiting requested review from disksing

@lhy1024 lhy1024 Awaiting requested review from lhy1024

@rleungx rleungx Awaiting requested review from rleungx

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.
7 participants
@JmPotato @rleungx @lhy1024 @HunDunDM @ti-srebot @nolouch @Yisaer