Skip to content

raftstore: write tombstone state with writes in one batch when catching up logs for merge [release-2.1]#4615

Merged
Connor1996 merged 4 commits into
tikv:release-2.1from
Connor1996:release-2.1
May 20, 2019
Merged

raftstore: write tombstone state with writes in one batch when catching up logs for merge [release-2.1]#4615
Connor1996 merged 4 commits into
tikv:release-2.1from
Connor1996:release-2.1

Conversation

@Connor1996

@Connor1996 Connor1996 commented Apr 30, 2019

Copy link
Copy Markdown
Member

What have you changed? (mandatory)

There are two bugs of #4581 and #4560 in master which fixed by #4595. But release-2.1 doesn't suffer from #4581, and to make changes as less as possible, this PR just write tombstone state with writes in one batch when catching up logs for merge instead of catching up logs through raft.

What are the type of the changes? (mandatory)

  • Bug fix (change which fixes an issue)

How has this PR been tested? (mandatory)

unit-test

@Connor1996

Copy link
Copy Markdown
Member Author

still have some compile errors, will be fixed later

@Connor1996 Connor1996 added S: WIP sig/raft Component: Raft, RaftStore, etc. labels May 6, 2019
@Connor1996

Copy link
Copy Markdown
Member Author

The change is a little large which is not stable enough to merge into the release branch, so finding an easy way to work around.

Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 changed the title [release-2.1] catch up logs for merge through raft raftstore: write tombstone state with writes in one batch when catching up logs for merge May 7, 2019
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
@Connor1996 Connor1996 added type/bugfix This PR fixes a bug. and removed S: WIP labels May 7, 2019
@Connor1996

Copy link
Copy Markdown
Member Author

PTAL @overvenus @BusyJay

@Connor1996 Connor1996 changed the title raftstore: write tombstone state with writes in one batch when catching up logs for merge raftstore: write tombstone state with writes in one batch when catching up logs for merge [release-2.1] May 7, 2019
Comment thread src/raftstore/store/peer.rs
Comment thread src/raftstore/store/worker/apply.rs Outdated
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Comment thread src/raftstore/store/worker/apply.rs
Comment thread src/raftstore/store/worker/apply.rs Outdated
fn should_write_to_engine(cmd: &RaftCmdRequest, wb_keys: usize) -> bool {
fn should_write_to_engine(cmd: &RaftCmdRequest, wb_keys: usize, for_merge_source: bool) -> bool {
if cmd.has_admin_request() {
match cmd.get_admin_request().get_cmd_type() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we guard this check with for_merge_source?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no, when handle committed entries for merge, it is still possible to handle some admin requests(not in log gap, just not applied in time) that should write to engine.

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

Copy link
Copy Markdown
Member Author

/test

@Connor1996

Copy link
Copy Markdown
Member Author

PTAL again @BusyJay @overvenus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/raft Component: Raft, RaftStore, etc. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants