allow raft apply committed logs before they are persisted #537

glorv · 2024-03-11T03:30:04Z

As committed means more than quorum node are persisted, which no data loss even if all other node are down. So in this situation, it is safe to apply this log even if it is still not persisted.

This PR introduces a new config max_applied_unpersisted_log_limit that allows return unpersisted raft log in light_ready. This is one step to optimize the tail latency that one slow node can significantly impact the overall latency. By setting a proper value for max_applied_unpersisted_log_limit we can avoid the in memory raft entries consumes too much memory which may lead to OOM.

After this change, if max_applied_unpersisted_log_limit is > 0, then it is possible: 1) applied > persisted. 2) applied > committed.(Only can happen at restart). So we loose some check since they are not always true anymore.

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

glorv · 2024-03-11T03:31:26Z

@Connor1996 @gengliqi @overvenus @tonyxuqqi @BusyJay PTAL, thank you~

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

cfzjywxk · 2024-03-11T08:08:14Z

Perhaps we could organize and link a top-down design document or RFC to illustrate the specific changes in tikv/raftstore, interface impacts, etc., after removing the implicit code constraints related to apply entry, as well as the corresponding design in raft-rs.

I remember @gengliqi has some relevant design documents before?

Connor1996 · 2024-03-13T12:03:55Z

src/raft.rs

@@ -366,6 +367,7 @@ impl<T: Storage> Raft<T> {
                max_committed_size_per_ready: c.max_committed_size_per_ready,
            },
        };
+        r.raft_log.apply_unpersisted_log_limit = c.apply_unpersisted_log_limit;


pass it through RaftLog::new?

src/raft_log.rs

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

Connor1996 · 2024-03-25T08:45:02Z

src/raft_log.rs

+    fn next_entries_upper_bound(&self) -> u64 {
+        std::cmp::min(
+            self.committed,
+            self.persisted + self.apply_unpersisted_log_limit,


how about passing log_limit from outside, like how max_size does. Then you can store the config inside RaftCore

tonyxuqqi · 2024-03-25T23:46:44Z

@glorv We should have a RFC document to summarize the design.

tonyxuqqi · 2024-03-25T23:58:06Z

src/config.rs

+
+    /// Maximum raft log number that can be applied after commit but before persist.
+    /// The default value is 0, which means apply after both commit and persist.
+    pub apply_unpersisted_log_limit: u64,


Will it support dynamic config update?

Yes, we provide a public function Raft::set_apply_unpersisted_log_limit to change this value in realtime.

gengliqi · 2024-03-28T06:18:30Z

src/config.rs

+
+    /// Maximum raft log number that can be applied after commit but before persist.
+    /// The default value is 0, which means apply after both commit and persist.
+    pub apply_unpersisted_log_limit: u64,


How about max_apply_unpersisted_log_limit?

gengliqi · 2024-03-28T07:07:48Z

src/raft_log.rs

+        // 1. then it is possible idx > persisted.
+        // 2. when the application restart after applied but before committed entried(and committed index) is persisted
+        //    then it is also possible idx > committed.
+        if idx < self.applied {


At least idx should be greater than or equal to self.committed.

This is for the initialization phase, currently it call applied_to to set the applied_index. I will change it to keep this check for normal path.

gengliqi · 2024-03-28T07:13:29Z

src/raft_log.rs

    pub persisted: u64,

    /// The highest log position that the application has been instructed
    /// to apply to its state machine.
    ///
-    /// Invariant: applied <= min(committed, persisted)
+    /// Invariant: applied <= min(committed, persisted + `apply_unpersisted_log_limit`)


apply_unpersisted_log_limit is a config that can be changed so it does not belong to Invariant.

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

…efore-persist

glorv · 2024-03-28T08:14:30Z

@Connor1996 @gengliqi PTAL again, thanks~

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

Connor1996

LGTM

gengliqi · 2024-03-28T09:15:57Z

src/raft_log.rs

@@ -42,20 +43,26 @@ pub struct RaftLog<T: Storage> {
    /// on a quorum of nodes.
    ///
    /// Invariant: applied <= committed
+    /// NOTE: this invariant can be break after restart if `max_apply_unpersisted_log_limit` is 0,


is greater than 0?

src/raft_log.rs

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

gengliqi · 2024-03-28T10:13:56Z

src/raft_log.rs

+    /// the maximum log gap between persisted_index and applied_index.
+    /// Caller should ensure the value won't lead to the upper bound overflow.


The comment about overflow may be unnecessary. Alternatively, it can be written in Config.

Suggested change

/// the maximum log gap between persisted_index and applied_index.

/// Caller should ensure the value won't lead to the upper bound overflow.

/// The maximum log gap between persisted and applied.

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

gengliqi · 2024-03-28T11:01:03Z

I think we need to add more restrictions to these logs that can be applied before persisting so that correctness errors can be found.
My idea of the restriction is:
A log can be applied before persisting only when a log with the same term has been committed and persisted.
After restarting, we can check if the logs between committed and applied have the same term as the last committed entry. If not the same, there must be something wrong and the panic must happen immediately.

gengliqi · 2024-03-28T11:15:04Z

I will propose another pr to implement this restriction.

allow raft apply committed logs before they are persisted

51a0ee5

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

glorv force-pushed the apply-before-persist branch from 7d8dc02 to 51a0ee5 Compare March 11, 2024 03:30

glorv added 5 commits March 11, 2024 13:03

fix

700838b

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

fix lint

06ad2e7

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

fix clippy for test

3cfd24e

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

fix lint

62777d0

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

fix lint

1fd38fe

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

glorv mentioned this pull request Mar 11, 2024

raftstore: allow apply committed logs before persist tikv/tikv#16626

Merged

9 tasks

Connor1996 reviewed Mar 13, 2024

View reviewed changes

resolve comment

4a7ee4c

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

Connor1996 reviewed Mar 25, 2024

View reviewed changes

tonyxuqqi reviewed Mar 25, 2024

View reviewed changes

gengliqi reviewed Mar 28, 2024

View reviewed changes

glorv added 3 commits March 28, 2024 16:02

resolve comments

ac7256e

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

do some rename

0b99c9e

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

Merge branch 'master' of https://github.com/tikv/raft-rs into apply-b…

d7bb11a

…efore-persist

fix test

ad21817

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

Connor1996 approved these changes Mar 28, 2024

View reviewed changes

gengliqi reviewed Mar 28, 2024

View reviewed changes

glorv added 2 commits March 28, 2024 17:24

fix comment

fe3c13c

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

rename

cc6e478

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

gengliqi reviewed Mar 28, 2024

View reviewed changes

unpdate doc comment

47c7f1d

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

glorv force-pushed the apply-before-persist branch from f66d2bb to 47c7f1d Compare March 28, 2024 10:30

gengliqi approved these changes Mar 28, 2024

View reviewed changes

gengliqi merged commit 3cfa667 into tikv:master Mar 28, 2024
5 checks passed

glorv deleted the apply-before-persist branch March 28, 2024 11:22

This was referenced Mar 28, 2024

Tracking issue for apply committed raft log before persistence tikv/tikv#16717

Open

fix applied upper bound #543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow raft apply committed logs before they are persisted #537

allow raft apply committed logs before they are persisted #537

glorv commented Mar 11, 2024 •

edited

Loading

glorv commented Mar 11, 2024 •

edited

Loading

cfzjywxk commented Mar 11, 2024

Connor1996 Mar 13, 2024

Connor1996 Mar 25, 2024

tonyxuqqi commented Mar 25, 2024

tonyxuqqi Mar 25, 2024

glorv Mar 26, 2024

gengliqi Mar 28, 2024

gengliqi Mar 28, 2024

glorv Mar 28, 2024

gengliqi Mar 28, 2024

glorv commented Mar 28, 2024

Connor1996 left a comment

gengliqi Mar 28, 2024

glorv Mar 28, 2024

gengliqi Mar 28, 2024

gengliqi commented Mar 28, 2024 •

edited

Loading

gengliqi commented Mar 28, 2024

		/// the maximum log gap between persisted_index and applied_index.
		/// Caller should ensure the value won't lead to the upper bound overflow.

allow raft apply committed logs before they are persisted #537

allow raft apply committed logs before they are persisted #537

Conversation

glorv commented Mar 11, 2024 • edited Loading

glorv commented Mar 11, 2024 • edited Loading

cfzjywxk commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyxuqqi commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glorv commented Mar 28, 2024

Connor1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengliqi commented Mar 28, 2024 • edited Loading

gengliqi commented Mar 28, 2024

glorv commented Mar 11, 2024 •

edited

Loading

glorv commented Mar 11, 2024 •

edited

Loading

gengliqi commented Mar 28, 2024 •

edited

Loading