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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for apply committed raft log before persistence #16717

Open
glorv opened this issue Mar 28, 2024 · 1 comment
Open

Tracking issue for apply committed raft log before persistence #16717

glorv opened this issue Mar 28, 2024 · 1 comment
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@glorv
Copy link
Contributor

glorv commented Mar 28, 2024

Development Task

Design: tikv/rfcs#112
@glorv glorv added the type/enhancement The issue or PR belongs to an enhancement. label Mar 28, 2024
This was referenced Mar 28, 2024
Merged
Merged
Merged
Merged
Merged
ti-chi-bot bot added a commit to tikv/pd that referenced this issue Apr 7, 2024
@glorv @ti-chi-bot @Connor1996
unsafe_recovery: also consider applied index when selects force leader (
569f3e0 
#7920)

close #7919, ref tikv/tikv#16717

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Co-authored-by: Connor <zbk602423539@gmail.com>
ti-chi-bot bot pushed a commit that referenced this issue Apr 9, 2024
@glorv
raftstore: allow apply committed logs before persist (#16626)
d5f681a 
ref #16717

This PR introduces a new raftstore config `max_apply_unpersisted_log_limit`. It means the max number of committed raft logs that can be applied before they are persisted. The default value is 0 to be compatible with current behavior.  In our benchmark, skip waiting log persistence can significantly optimize the tail latency that when the persistence of raft log is quite slow on minor instances.

After this change the invariant that both `applied_index <= persisted_index` and `applied_index <= committed_index` will not be true anymore, so we will loose some checks on this.

To make the implementation simple, we add some restriction that in theory is unnecessary:
- Only apply unpersisted logs on region leader. We may also support this to follower/learner in the future if the mechanism is proved effective. Then features such as "stale read" and tiflash can also benefit from it.
- Only apply unpersisted logs when the raft term is unchanged. This is to help verify that once some logs are applied but not persist, we can use this term to verify that the resynced logs are unchanged.
- Do not  apply unpersisted logs for PrepareMerge. This can make compatible with `online unafe recovery` much easier.

In order to support these restriction, the raft fsm will auto disable this feature(by setting apply_unpersisted_log_limit = 0) and auto recover on certain events.

This PR also do a small change to raft batch commend by allowing propose if all ongoing commends are committed, this is to help alleviate the tail latency when there are io jitters.

NOTE: After this PR, because applied_index can be larger than committed index, it may not be compatible with online unsafe recovery anymore. We will handle this in a separate PR later.

Signed-off-by: glorv <glorvs@163.com>
ti-chi-bot bot added a commit that referenced this issue Apr 11, 2024
@glorv @ti-chi-bot
raftstore: make apply before persist compatible with online unsafe re…
57615b2 
…covery (#16651)

ref #16717, close #16796

Before enter force-leader, if local applied_index > last_index, first force commit and truncate raft index to applied index. This can ensure the raft state is compatible with apply_state.

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
@ti-chi-bot ti-chi-bot mentioned this issue Apr 11, 2024
Merged
9 tasks
ti-chi-bot bot pushed a commit that referenced this issue Apr 11, 2024
@ti-chi-bot @glorv
raftstore: make apply before persist compatible with online unsafe re…
b7103fa 
…covery (#16651) (#16801)

ref #16717, close #16796

Before enter force-leader, if local applied_index > last_index, first force commit and truncate raft index to applied index. This can ensure the raft state is compatible with apply_state.

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: glorv <glorvs@163.com>
This was referenced Apr 12, 2024
Merged
Merged
ti-chi-bot bot pushed a commit that referenced this issue Apr 15, 2024
@glorv @tonyxuqqi
add grafana panel for applying unpersisted log (#16821)
92ee4d2 
ref #16717, close #16820

- Change `RAFT_DISABLE_UNPERSISTED_APPLY_GAUGE` to `RAFT_ENABLE_UNPERSISTED_APPLY_GAUGE` because it's hard to calculate the previous correctly.
- Add two new grafana panels for "Apply unpersisted raft logs"

Co-authored-by: tonyxuqqi <tonyxuqi@outlook.com>
@ti-chi-bot ti-chi-bot mentioned this issue Apr 16, 2024
Merged
9 tasks
ti-chi-bot bot pushed a commit that referenced this issue Apr 16, 2024
@ti-chi-bot @glorv
add grafana panel for applying unpersisted log (#16821) (#16842)
4099d0f 
ref #16717, close #16820

- Change `RAFT_DISABLE_UNPERSISTED_APPLY_GAUGE` to `RAFT_ENABLE_UNPERSISTED_APPLY_GAUGE` because it's hard to calculate the previous correctly.
- Add two new grafana panels for "Apply unpersisted raft logs"

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: glorv <glorvs@163.com>
ti-chi-bot bot added a commit that referenced this issue Apr 22, 2024
@glorv @ti-chi-bot
raftstore: enable apply committed log before persistence by default (#…
cbda550 
…16834)

ref #16717

Change the default value of `raftstore.max_apply_unpersisted_log_limit` from 0 to 1024 to enable apply committed log before persistence by default.

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
@glorv glorv mentioned this issue Apr 25, 2024
Open
9 tasks
@glorv
Copy link
Contributor Author

glorv commented May 23, 2024

Tracking issue for the potential OOM cause by this feature: #17055

@glorv glorv mentioned this issue May 23, 2024
Merged
9 tasks
ti-chi-bot bot pushed a commit that referenced this issue May 23, 2024
@glorv
raftstore: do not free cached entries if not all of them are persisted (
fba1fb2 
#17050)

ref #16717, close #17040

Do not free cached entries if not all of them are persisted. After #16834, it is possible that applying entries are not persisted, so we should avoid evicting these entries while they are not fully persisted.

Signed-off-by: glorv <glorvs@163.com>
@zhangjinpeng87 zhangjinpeng87 mentioned this issue Jun 26, 2024
Merged
18 tasks
@ti-chi-bot ti-chi-bot mentioned this issue Jun 27, 2024
Merged
9 tasks
ti-chi-bot bot pushed a commit that referenced this issue Jun 27, 2024
@ti-chi-bot @glorv
raftstore: do not free cached entries if not all of them are persisted (
675e6dd 
#17050) (#17207)

ref #16717, close #17040

Do not free cached entries if not all of them are persisted. After #16834, it is possible that applying entries are not persisted, so we should avoid evicting these entries while they are not fully persisted.

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: glorv <glorvs@163.com>
RidRisR pushed a commit to RidRisR/tikv that referenced this issue Aug 5, 2024
@ti-chi-bot @glorv @RidRisR
raftstore: do not free cached entries if not all of them are persisted (
304987b 
tikv#17050) (tikv#17207)

ref tikv#16717, close tikv#17040

Do not free cached entries if not all of them are persisted. After tikv#16834, it is possible that applying entries are not persisted, so we should avoid evicting these entries while they are not fully persisted.

Signed-off-by: glorv <glorvs@163.com>

Co-authored-by: glorv <glorvs@163.com>
Signed-off-by: RidRisR <79858083+RidRisR@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@glorv