raftstore-v2: compact and gc raft logs by tabokie · Pull Request #13846 · tikv/tikv

tabokie · 2022-11-25T04:56:00Z

Signed-off-by: tabokie [email protected]

What is changed and how it works?

Issue Number: Ref #12842

What's Changed:

Related changes

Check List

Tests

Unit test

Release note

None

Signed-off-by: tabokie <[email protected]>

ti-chi-bot · 2022-11-25T04:56:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

BusyJay
Connor1996

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: tabokie <[email protected]>

components/raftstore-v2/src/worker/raft_log_gc.rs

Signed-off-by: tabokie <[email protected]>

components/raftstore-v2/src/operation/command/admin/compact_log.rs

Signed-off-by: tabokie <[email protected]>

BusyJay · 2022-12-06T18:17:25Z

Logs can't be gced until its data are all applied and flushed. So log gc depends on removing WAL. I suggest to continue the work after removing wal is supported.

components/raftstore-v2/src/operation/raft_log_gc.rs

…tlog-gc Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

components/raftstore-v2/src/operation/raft_log_gc.rs

components/raftstore-v2/src/operation/command/admin/compact_log.rs

Signed-off-by: tabokie <[email protected]>

BusyJay · 2022-12-23T10:56:06Z

components/raftstore/src/store/worker/raftlog_gc.rs

-                RAFT_LOG_GC_DELETED_KEYS_HISTOGRAM.observe(n as f64);
-            }
+        }
+        if let Err(e) = self.engines.raft.consume(&mut batch, false) {


Maybe you can keep the most of original flush by:

fn gc_raft_log() { ... consume(batch) ... } fn flush() { ... for _ in _ { engine.gc(batch); } ... }

So most code is unchanged and easier to understand.

If it's the failpoint bothering you, the old code is simply wrong. It skips writing log batch but doesn't skip consuming callbacks. It is not the intended behavior as per

tikv/tests/failpoints/cases/test_snap.rs

Lines 701 to 702 in 8864e9b

// Simulate raft log gc are pending in queue.

let fp = "worker_gc_raft_log";

It's actually the expected behavior. Because we just ignore any write errors and only prints a log. The callbacks are used to ensure no more background writes to the same region.

Regardless of the behavior it is not what the test comment says, because tasks are lost forever. For RocksDB GC it means some entries will never be compacted again. So we will change the failpoint comment if that's what you prefer.

The return action is to simulate the case that the node is stopped before completing GC. It's more appropriate to use "pause", but "pause" will stop the case from restarting node.

On the other hand, writing raft engine can fail. If that error happens, we just ignore it for now. I think they are two different things.

components/raftstore/src/store/mod.rs

BusyJay · 2022-12-23T10:59:46Z

components/raftstore/src/store/fsm/peer.rs

            self.fsm.peer.mut_store().evict_entry_cache(true);
-        }
-        let mut _usage = 0;
-        if memory_usage_reaches_high_water(&mut _usage)


Why the change?

In the next tick it will call needs_evict_entry_cache again, which calls memory_usage_reaches_high_water inside. I think checking later again makes more sense than checking right after a compaction.

Also, this further hides the function memory_usage_reaches_high_water and keeps the pattern consistent with

tikv/components/raftstore/src/store/fsm/peer.rs

Lines 5350 to 5355 in 8864e9b

if needs_evict_entry_cache(self.ctx.cfg.evict_cache_on_memory_ratio) {

self.fsm.peer.mut_store().evict_entry_cache(true);

if !self.fsm.peer.get_store().is_entry_cache_empty() {

self.register_entry_cache_evict_tick();

}

}

BusyJay · 2022-12-23T11:04:52Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+            self.entry_storage().truncated_index(),
+            self.storage().apply_trace().persisted_apply_index(),
+        );
+        if compact_index > self.last_engine_compact_log_index() {


last_engine_compact_log_index may not be necessary. For on_ready_res, it should compact log if truncated_index < persisted_applied before applying the updates, For record_apply_trace, it should compact log if persisted_applied < truncated_index.

~~You are right. But it makes the code a little more complex, and it introduces an extra dependency between caller and callee.~~

It wouldn't work if one index advances across another index,

Why? For example, truncated_index is advanced from 3 to 10 in on_ready_res, while persisted_applied is 5. Because 3 < 5, so it should gc logs using cmp::min(10, 5) = 5.

I see. It is still very complex.

It's just two if check to get rid of a field (and two if check).

Signed-off-by: tabokie <[email protected]>

…tlog-gc Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

BusyJay · 2022-12-26T05:43:50Z

components/raft_log_engine/src/engine.rs

+                                if found_flush_state[cf_id as usize] {
+                                    batch.0.delete(raft_group_id, key.to_vec());
+                                } else {
+                                    found_flush_state[cf_id as usize] = true;


Will IndexOutOfBound if cf_id == MAX_CF_ID.

BusyJay · 2022-12-26T05:48:45Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+                .and_then(|_| {
+                    store_ctx
+                        .engine
+                        .delete_all_but_one_states_before(region_id, compact_index, lb)


I don't get it. It should delete those states whenever persisted_applied is advanced. For example, even the truncated index is 3, but it can delete the states at 6 when applied index is 10 and there is another state at 7.

BusyJay · 2022-12-26T05:57:38Z

components/raftstore/src/store/worker/raftlog_gc.rs

-                RAFT_LOG_GC_DELETED_KEYS_HISTOGRAM.observe(n as f64);
-            }
+        }
+        if let Err(e) = self.engines.raft.consume(&mut batch, false) {


It's actually the expected behavior. Because we just ignore any write errors and only prints a log. The callbacks are used to ensure no more background writes to the same region.

Signed-off-by: tabokie <[email protected]>

BusyJay

Rest LGTM

BusyJay · 2022-12-26T07:52:25Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+        }
+
+        let data = req.write_to_bytes().unwrap();
+        self.propose_with_ctx(store_ctx, data, vec![])


Suggested change

self.propose_with_ctx(store_ctx, data, vec![])

self.propose(store_ctx, data)

BusyJay · 2022-12-26T07:55:00Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+        }
+        // TODO: check is_merging
+        // compact failure is safe to be omitted, no need to assert.
+        if res.compact_index <= self.entry_storage().truncated_index() {


This is covered by checking first_index.

BusyJay · 2022-12-26T07:56:10Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+            .apply_state()
+            .get_truncated_state()
+            .get_index();
+        self.entry_storage_mut()


Save apply_state_mut as a temporary can reduce code.

BusyJay · 2022-12-26T07:57:38Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+                self.set_has_extra_write();
+            }
+        }
+        self.maybe_compact_log_from_engine(store_ctx, Either::Left(old_persisted));


I think it should be included in if above.

BusyJay · 2022-12-26T07:59:08Z

components/raftstore-v2/src/operation/command/admin/compact_log.rs

+            Either::Right(old_truncated) if old_truncated >= persisted => return,
+            _ => {}
+        }
+        let compact_index = std::cmp::min(


You mean let compact_index = std::cmp::min(truncated, persisted)?

BusyJay · 2022-12-26T08:02:29Z

components/raftstore-v2/src/raft/peer.rs

+        if let Some(last_heartbeat) = self.peer_heartbeats.get(&peer_id)
+            && *last_heartbeat >= *deadline
+        {
+            return true;
+        }
+        false
+    }


You mean matches!(self.peer_heartbeats.get(&peer_id), Some(last_heartbat) if *last_heartbeat >= *deadline)?

BusyJay · 2022-12-26T08:04:06Z

components/raftstore/src/store/worker/raftlog_gc.rs

-            }
        }
-        RAFT_LOG_GC_WRITE_DURATION_HISTOGRAM.observe(start.saturating_elapsed_secs());
-        for cb in cbs {


Callback are not collected and invoked.

Signed-off-by: tabokie <[email protected]>

Connor1996

LGTM

…tlog-gc Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

tabokie · 2022-12-27T03:28:07Z

Latest commit needs review. @bufferflies @BusyJay

tabokie · 2022-12-27T04:35:22Z

/merge

ti-chi-bot · 2022-12-27T04:35:23Z

@tabokie: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-12-27T04:35:26Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 97f9300

add the basic skeleton

fca8680

Signed-off-by: tabokie <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 25, 2022

tabokie added 3 commits November 25, 2022 14:06

add driving logic

432bf10

Signed-off-by: tabokie <[email protected]>

add implementation

077bcc0

Signed-off-by: tabokie <[email protected]>

tick on start

d192187

Signed-off-by: tabokie <[email protected]>

tabokie requested review from BusyJay, Connor1996 and tonyxuqqi November 25, 2022 06:53

BusyJay reviewed Nov 25, 2022

View reviewed changes

components/raftstore-v2/src/worker/raft_log_gc.rs Outdated Show resolved Hide resolved

add checks

1620f01

Signed-off-by: tabokie <[email protected]>

tabokie commented Nov 25, 2022

View reviewed changes

components/raftstore-v2/src/operation/command/admin/compact_log.rs Outdated Show resolved Hide resolved

tabokie added 2 commits November 28, 2022 11:15

fix some bugs

8f579b1

Signed-off-by: tabokie <[email protected]>

fix bugs

bd60a46

Signed-off-by: tabokie <[email protected]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2022

tonyxuqqi reviewed Dec 20, 2022

View reviewed changes

components/raftstore-v2/src/operation/raft_log_gc.rs Outdated Show resolved Hide resolved

tonyxuqqi reviewed Dec 20, 2022

View reviewed changes

components/raftstore-v2/src/operation/raft_log_gc.rs Outdated Show resolved Hide resolved

tabokie added 2 commits December 20, 2022 18:42

Merge branch 'master' of https://github.com/tikv/tikv into 221125-raf…

5fc7d6a

…tlog-gc Signed-off-by: tabokie <[email protected]>

renames

598ce85

Signed-off-by: tabokie <[email protected]>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

clean up

da11a21

Signed-off-by: tabokie <[email protected]>

BusyJay reviewed Dec 20, 2022

View reviewed changes

tabokie added 4 commits December 21, 2022 11:44

address comments

e11c902

Signed-off-by: tabokie <[email protected]>

fix visibility

5e7542c

Signed-off-by: tabokie <[email protected]>

rename

3e96670

Signed-off-by: tabokie <[email protected]>

add some comments

e44d9e7

Signed-off-by: tabokie <[email protected]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2022

use one single worker for all engine gc tasks

7cf3b2b

Signed-off-by: tabokie <[email protected]>

BusyJay reviewed Dec 23, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 24, 2022

tabokie added 2 commits December 26, 2022 12:20

address comments

e84afa4

Signed-off-by: tabokie <[email protected]>

Merge branch 'master' of https://github.com/tikv/tikv into 221125-raf…

95cf5ef

…tlog-gc Signed-off-by: tabokie <[email protected]>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 26, 2022

remove a wrong callsite of maybe compact

9c60cdd

Signed-off-by: tabokie <[email protected]>

BusyJay reviewed Dec 26, 2022

View reviewed changes

tabokie added 2 commits December 26, 2022 14:53

address comments

6592c3d

Signed-off-by: tabokie <[email protected]>

address comment

d744206

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the 221125-raftlog-gc branch from 83d2930 to d744206 Compare December 26, 2022 07:25

BusyJay reviewed Dec 26, 2022

View reviewed changes

address comments

4c3dad3

Signed-off-by: tabokie <[email protected]>

BusyJay approved these changes Dec 26, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 26, 2022

Connor1996 approved these changes Dec 26, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 26, 2022

tabokie added 2 commits December 27, 2022 11:17

Merge branch 'master' of https://github.com/tikv/tikv into 221125-raf…

870a1e1

…tlog-gc Signed-off-by: tabokie <[email protected]>

fix cancel snap conflict

97f9300

Signed-off-by: tabokie <[email protected]>

BusyJay approved these changes Dec 27, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 27, 2022

ti-chi-bot merged commit f21361d into tikv:master Dec 27, 2022

ti-chi-bot added this to the Pool milestone Dec 27, 2022

tabokie deleted the 221125-raftlog-gc branch December 27, 2022 04:46

hbisheng mentioned this pull request May 19, 2025

Reduce unnecessary EntryCacheEvict ticks in Raftstore #18462

Closed

	// Simulate raft log gc are pending in queue.
	let fp = "worker_gc_raft_log";

	if needs_evict_entry_cache(self.ctx.cfg.evict_cache_on_memory_ratio) {
	self.fsm.peer.mut_store().evict_entry_cache(true);
	if !self.fsm.peer.get_store().is_entry_cache_empty() {
	self.register_entry_cache_evict_tick();
	}
	}

	self.propose_with_ctx(store_ctx, data, vec![])
	self.propose(store_ctx, data)

Conversation

tabokie commented Nov 25, 2022

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BusyJay commented Dec 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabokie Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

tabokie commented Dec 27, 2022

Uh oh!

tabokie commented Dec 27, 2022

Uh oh!

ti-chi-bot commented Dec 27, 2022

ti-chi-bot commented Nov 25, 2022 •

edited

Loading

tabokie Dec 26, 2022 •

edited

Loading

BusyJay Dec 26, 2022 •

edited

Loading