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.

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

raftstore: not always update pending_cross_snap #3822

Merged
merged 17 commits into from Dec 14, 2018

Conversation

Projects
None yet
5 participants
@Connor1996
Copy link
Member

commented Nov 22, 2018

What have you changed? (mandatory)

pending_cross_snap is updated when a new message is received or a snapshot is received. However, need_gc_merge assumes an updated field indicates a snapshot is received. This can lead to wrong decision which may ruin merge process that one peer destroy itself, but the other peer want to merge it.

This PR fixes it by checking the range is covered by exist regions to decide whether updating pending_cross_snap when a new message is received. If that, means that the region may generated by some kinds of split and merge by catching logs. So it is no need to accept snapshot, namely, do not need to update pending_cross_snap.

What are the type of the changes? (mandatory)

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit-test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Connor1996 added some commits Nov 22, 2018

add test
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
check range covered
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 requested a review from BusyJay Nov 22, 2018

@Connor1996 Connor1996 changed the title raftstore: update pending_cross_snap raftstore: not always update pending_cross_snap Nov 22, 2018

Connor1996 added some commits Nov 22, 2018

clean
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
make format
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 requested review from overvenus and hicqu Nov 22, 2018

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

PTAL @BusyJay

@@ -470,10 +470,11 @@ impl<T: Transport, C: PdClient> Store<T, C> {
.map(|p| p.region().get_region_epoch())
}) {
info!(
"[region {}] checking target {} epoch: {:?}",
"[region {}] checking target {} epoch: {:?}, msg target epoch: {:?}",

This comment has been minimized.

Copy link
@overvenus

overvenus Nov 26, 2018

Member

merge target?

This comment has been minimized.

Copy link
@Connor1996

Connor1996 Nov 27, 2018

Author Member

here is to indicate merge target from message to distinguish with previous checking target

Show resolved Hide resolved tests/raftstore_cases/test_merge.rs Outdated

Connor1996 added some commits Nov 26, 2018

fix typo
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
add more test
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 force-pushed the Connor1996:check-merge branch from 532f143 to b0d93ff Nov 26, 2018

fix format
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 force-pushed the Connor1996:check-merge branch from b0d93ff to 08db432 Nov 27, 2018

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Connor1996 added some commits Nov 30, 2018

address comment
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
fix grammar
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
address comment
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@BusyJay
Copy link
Contributor

left a comment

I don't see how the test case works.

Connor1996 added some commits Nov 30, 2018

add comment
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
add comment
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
make format
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
fix typo
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 force-pushed the Connor1996:check-merge branch from 2401fbc to f4c3e6b Dec 4, 2018

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

PTAL

@BusyJay
Copy link
Contributor

left a comment

rest LGTM

Show resolved Hide resolved src/raftstore/store/fsm/store.rs Outdated
Show resolved Hide resolved tests/integrations/raftstore/test_merge.rs

@Connor1996 Connor1996 force-pushed the Connor1996:check-merge branch from 8613407 to 3a28c92 Dec 5, 2018

address comment
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 force-pushed the Connor1996:check-merge branch from 3a28c92 to d541a73 Dec 5, 2018

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@zhangjinpeng1987
Copy link
Member

left a comment

LGTM

@Connor1996 Connor1996 merged commit ed635e2 into tikv:master Dec 14, 2018

4 checks passed

DCO All commits are signed off!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details

@Connor1996 Connor1996 deleted the Connor1996:check-merge branch Dec 14, 2018

Connor1996 added a commit to Connor1996/tikv that referenced this pull request Dec 17, 2018

Connor1996 added a commit to Connor1996/tikv that referenced this pull request Dec 17, 2018

raftstore: not always update pending_cross_snap (tikv#3822)
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

Connor1996 added a commit that referenced this pull request Dec 18, 2018

raftstore: not always update pending_cross_snap (#3822) (#3940)
Signed-off-by: Connor1996 <zbk602423539@gmail.com>

overvenus added a commit that referenced this pull request Jan 2, 2019

raftstore: not always update pending_cross_snap (#3822) (#4003)
* raftstore: do not update pending_cross_snap when peer is applying snapshot(#3873)

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

@Connor1996 Connor1996 added the T: BugFix label Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.