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

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
ti-chi-bot bot added a commit to tikv/pd that referenced this issue Apr 7, 2024
#7920)

close #7919, ref tikv/tikv#16717

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

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Co-authored-by: Connor <[email protected]>
ti-chi-bot bot pushed a commit that referenced this issue Apr 9, 2024
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 <[email protected]>
ti-chi-bot bot added a commit that referenced this issue Apr 11, 2024
…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 <[email protected]>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ti-chi-bot bot pushed a commit that referenced this issue Apr 11, 2024
…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 <[email protected]>

Co-authored-by: glorv <[email protected]>
ti-chi-bot bot pushed a commit that referenced this issue Apr 15, 2024
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 <[email protected]>
ti-chi-bot bot pushed a commit that referenced this issue Apr 16, 2024
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 <[email protected]>

Co-authored-by: glorv <[email protected]>
ti-chi-bot bot added a commit that referenced this issue Apr 22, 2024
…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 <[email protected]>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
@glorv
Copy link
Contributor Author

glorv commented May 23, 2024

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

ti-chi-bot bot pushed a commit that referenced this issue May 23, 2024
#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 <[email protected]>
ti-chi-bot bot pushed a commit that referenced this issue Jun 27, 2024
#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 <[email protected]>

Co-authored-by: glorv <[email protected]>
RidRisR pushed a commit to RidRisR/tikv that referenced this issue Aug 5, 2024
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 <[email protected]>

Co-authored-by: glorv <[email protected]>
Signed-off-by: RidRisR <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

No branches or pull requests

1 participant