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

Fix commit index is not forwarded when merge entry is empty #5512

Merged
merged 11 commits into from Sep 24, 2019

Conversation

@Connor1996
Copy link
Member

commented Sep 23, 2019

When merge entry is empty, it is possible that the source peer has caught up the logs but the commit index is not updated in time.
Due to other source peers may be already destroyed, so catch up log process should forward commit index even if merge entry is empty.

Fix pingcap/tidb#12290

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2019

/release

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2019

/release

@Connor1996 Connor1996 force-pushed the Connor1996:add-log branch 2 times, most recently from 6b26a7b to 39d3faa Sep 24, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 force-pushed the Connor1996:add-log branch from 39d3faa to 8cb2c9b Sep 24, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 changed the title [DNM] Fix merge empty entry [DNM] Fix commit index is not forwarded when merge entry is empty Sep 24, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 changed the title [DNM] Fix commit index is not forwarded when merge entry is empty Fix commit index is not forwarded when merge entry is empty Sep 24, 2019
fail::cfg("before_set_applied_index_1000_1003", "return()").unwrap();
fail::cfg("apply_before_prepare_merge_1000_1003", "return()").unwrap();
pd_client.must_merge(left.get_id(), right.get_id());
thread::sleep(Duration::from_millis(100));

This comment has been minimized.

Copy link
@BusyJay

BusyJay Sep 24, 2019

Contributor

It is not necessary to use failpoint to test this case. It should be sufficient to just construct a cluster with two peers merged, one peer has enough logs but still not committed, and start it to see if it can still be merged.

Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 force-pushed the Connor1996:add-log branch from 77ff02b to b3d7b96 Sep 24, 2019
Connor1996 added 2 commits Sep 24, 2019
clean
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 force-pushed the Connor1996:add-log branch from a9b4d4a to cff238d Sep 24, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Copy link
Contributor

left a comment

rest LGTM

.direction(Direction::Recv),
));
pd_client.must_merge(left.get_id(), right.get_id());
thread::sleep(Duration::from_millis(100));

This comment has been minimized.

Copy link
@BusyJay

BusyJay Sep 24, 2019

Contributor

I think you can use must_region_not_exist to make the test more stable.

This comment has been minimized.

Copy link
@Connor1996

Connor1996 Sep 24, 2019

Author Member

it can be just removed, must_merge will wait

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

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2019

/test

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

left a comment

LGTM

@@ -457,11 +457,22 @@ impl Peer {
pub fn maybe_append_merge_entries(&mut self, merge: &CommitMergeRequest) -> Option<u64> {
let mut entries = merge.get_entries();
if entries.is_empty() {
return None;
if merge.get_commit() > self.raft_group.raft.raft_log.committed {

This comment has been minimized.

Copy link
@overvenus

overvenus Sep 24, 2019

Contributor

Could you add some comments about why we can forward the commit index?

Connor1996 added 2 commits Sep 24, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 merged commit 9b59dee into tikv:master Sep 24, 2019
4 checks passed
4 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

cherry pick to release-3.0 failed

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

cherry pick to release-3.1 failed

@Connor1996

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

/cherry-picker

Connor1996 added a commit to Connor1996/tikv that referenced this pull request Sep 25, 2019
* fix merge empty entry check

Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Connor1996 added a commit to Connor1996/tikv that referenced this pull request Sep 25, 2019
* fix merge empty entry check

Signed-off-by: Connor1996 <zbk602423539@gmail.com>
sre-bot added a commit that referenced this pull request Sep 25, 2019
sre-bot added a commit that referenced this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.