raftstore: Implement coprocessor observer pre_persist #12957
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
release-note-none
ti-chi-bot
added
size/L
ti-chi-bot
added
the
needs-rebase
ti-chi-bot
added
size/M
|
/cc @tonyxuqqi @BusyJay |
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ti-chi-bot
added
the
needs-rebase
ti-chi-bot
removed
the
needs-rebase
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ti-chi-bot
added
size/L
| /// For example, in `finish_for` and `commit`, | ||
| /// we will separately call `pre_commit` with is_finished = true/false. | ||
| /// By returning false, we reject this persistence. | ||
| pub fn pre_commit( |
There was a problem hiding this comment.
Better give it a better name without leaking the implementation details. It will be changed in v2.
| @@ -269,6 +269,7 @@ impl Simulator for NodeCluster { | |||
| .max_total_size(cfg.server.snap_max_total_size.0) | |||
| .encryption_key_manager(key_manager) | |||
| .max_per_file_size(cfg.raft_store.max_snapshot_file_raw_size.0) | |||
| .enable_multi_snapshot_files(true) | |||
There was a problem hiding this comment.
Because I find test cases like test_server_huge_snapshot_multi_files may not check expected behavior if enable_multi_snapshot_files is set to false.
These tests are to test if snapshot works fine when we enable multi file snapshot. However, we may not generate multi file snapshot if this switch is closed, so these tests will actually test single file snapshot.
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
…ext into merge-tikv-commit
ti-chi-bot
added
status/LGT2
|
/merge |
|
@breezewish: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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. |
|
This pull request has been accepted and is ready to merge. Commit hash: b57d9e8
|
ti-chi-bot
added
the
status/can-merge
CalvinNeo
changed the title
|
/run-all-tests |
ref tikv#12849 Support coprocessor observer pre_commit Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ref tikv#12849 Support coprocessor observer pre_commit Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ref tikv#12849 Support coprocessor observer pre_commit Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ref tikv#12849 Support coprocessor observer pre_commit Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ref tikv#12849 Support coprocessor observer pre_commit Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
Signed-off-by: CalvinNeo calvinneo1995@gmail.com
What is changed and how it works?
Issue Number: ref #12849
What's Changed:
Before commit and finish_for, if the observer decides it is not a proper time to do actual persist, it can skip this persistence.
This observer is aim to disable persistence of advanced
applied_index, since some KvEngine may not persist replicated data whencommitandfinish_for, so we can persist advancedapplied_indexeither.This can be easily done if we just disable
write_apply_state. However, TiKV assert that data and meta should be persist as a whole, so if we don't want to persist advancedapplied_index, we should not persist data(which is actually empty, except some pending deleting ssts) either.We don't worry too much about rubbish data. Since commit can also be triggered by
post_exec, or at leastApplyContext::end. We don't callwrite_apply_sateinApplyContext::end.Related changes
pingcap/docs/pingcap/docs-cn:Check List
Tests
Side effects
Release note