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

server/core: check region term before updating cache #2667

Merged
merged 6 commits into from
Jul 22, 2020
Merged

server/core: check region term before updating cache #2667

merged 6 commits into from
Jul 22, 2020

Conversation

howardlau1999
Copy link
Contributor

Signed-off-by: howardlau1999 [email protected]

What problem does this PR solve?

Close #2657

What is changed and how it works?

Collect the term field in the heartbeat message into RegionInfo and check if the term is greater than the one in cache to avoid invalid cache updates.

Check List

Tests

  • Unit test

Code changes

  • Has PreCheckPutRegion changed
  • Add field term in struct RegionInfo

Release note

Avoid invalid cache updates after the leader of a region has changed.

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Jul 20, 2020
@CLAassistant
Copy link

CLAassistant commented Jul 20, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@howardlau1999 howardlau1999 force-pushed the check-term branch 2 times, most recently from 4aef910 to 53940fd Compare July 20, 2020 09:25
@disksing
Copy link
Contributor

When did TiKV support sending term in heartbeat message? Do we need to consider backward compatibility problem?

@howardlau1999
Copy link
Contributor Author

howardlau1999 commented Jul 20, 2020 •
edited
Loading

TiKV support sending term in heartbeat message after v3.0.(tikv/tikv#5281) It may cause problems if TiKV older than v3.0 and newer than v3.0 are running at the same time. Heartbeat messages from old versions do not contain term which will default to 0 so the check will always fail if old versions TiKV heartbeats after new versions. Only heartbeats from new versions TiKV can update the cache. And if all new TiKV fail, the old versions TiKV will constantly fail to update the cache.

If all TiKVs do not contain term, then this PR has no effect because the term will default to zero all the time.

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

/rebuild

JmPotato
JmPotato approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

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

@JmPotato,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: scheduling(slack).

@nolouch nolouch added needs-cherry-pick-release-3.0 The PR needs to cherry pick to release-3.0 branch. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. labels Jul 21, 2020
@howardlau1999
server/core: clone term field
a5c45b9 
Signed-off-by: howardlau1999 <[email protected]>
nolouch
nolouch approved these changes Jul 21, 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 Jul 21, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 21, 2020
@nolouch
Copy link
Contributor

nolouch commented Jul 21, 2020

/merge

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

/run-all-tests

@nolouch
Copy link
Contributor

nolouch commented Jul 21, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@howardlau1999 merge failed.

@nolouch
Copy link
Contributor

nolouch commented Jul 22, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit a9cdb2d into tikv:master Jul 22, 2020
ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Jul 22, 2020
@howardlau1999 @ti-srebot
cherry pick tikv#2667 to release-3.0
861771b 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jul 22, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #2671

ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Jul 22, 2020
@howardlau1999 @ti-srebot
cherry pick tikv#2667 to release-4.0
0794d01 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jul 22, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #2672

howardlau1999 added a commit to howardlau1999/pd that referenced this pull request Jul 23, 2020
@howardlau1999
server/core: cherry-pick tikv#2667
7812529 
Signed-off-by: Howard Lau <[email protected]>
ti-srebot added a commit that referenced this pull request Jul 27, 2020
@ti-srebot
server/core: check region term before updating cache (#2667) (#2672)
2786acc 
Signed-off-by: ti-srebot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@disksing disksing disksing approved these changes

@nolouch nolouch nolouch approved these changes

@JmPotato JmPotato JmPotato approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. needs-cherry-pick-release-3.0 The PR needs to cherry pick to release-3.0 branch. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. 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.

Consider check the term when update the leader
6 participants
@howardlau1999 @CLAassistant @disksing @JmPotato @ti-srebot @nolouch