raftstore: Implement engine trait can_apply_snapshot #12924

CalvinNeo · 2022-06-28T05:55:29Z

What is changed and how it works?

Issue Number: ref #12849

What's Changed:

Support new engine trait can_apply_snapshot

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

None

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

ti-chi-bot · 2022-06-28T05:55:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

BusyJay
tonyxuqqi

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.

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

components/engine_rocks/src/engine.rs

components/engine_traits/src/engine.rs

components/engine_rocks/src/raw_util.rs

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

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

CalvinNeo · 2022-06-30T04:55:32Z

/run-all-tests

CalvinNeo · 2022-06-30T16:06:12Z

/run-all-tests

CalvinNeo · 2022-07-11T01:45:23Z

/run-all-tests

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

CalvinNeo · 2022-08-22T02:27:22Z

/run-all-tests

CalvinNeo · 2022-08-22T05:38:51Z

/cc @BusyJay

BusyJay · 2022-08-22T06:00:20Z

components/engine_rocks/src/engine.rs

@@ -66,6 +66,10 @@ impl RocksEngine {
        self.shared_block_cache = enable;
    }

+    pub fn get_shared_block_cache(&self) -> bool {


Should be shared_block_cache. Note we will remove the field in the future and cache will be always shared.

BusyJay · 2022-08-22T06:03:53Z

components/engine_traits/src/engine.rs

+    /// Some KvEngines need to do some transforms before apply data from
+    /// snapshot. These procedures can be batched in background if there are
+    /// more than one incoming snapshots, thus not blocking applying thread.
+    fn can_apply_snapshot(&self, _is_timeout: bool, _region_id: u64) -> bool {


Why need is_timeout? How can it make any difference?

If there is no incoming applying snapshot for a long time, handle_pending_applies can be triggered by on_timeout, thus proxy can do it directly in foreground.

Then why bother calling can_apply_snapshot again when timeout?

handle_pending_applies will apply multiple snapshots as long as it will not cause write stall. However, in proxy we won't handle the more than one snapshot if the following snapshot is still pre-handling, thus avoid continuous blocking of the thread for applying snapshot.

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

…-trait

CalvinNeo · 2022-09-07T02:08:57Z

/run-all-tests

CalvinNeo · 2022-09-07T03:33:57Z

/cc @breezewish

breezewish · 2022-09-08T02:22:16Z

The rest looks good to me. @tonyxuqqi PTAL, thanks!

components/raftstore/src/store/config.rs

tonyxuqqi · 2022-09-08T04:14:42Z

components/raftstore/src/store/peer_storage.rs

+    const PENDING_APPLY_CHECK_INTERVAL: u64 = 200;
+    const STALE_PEER_CHECK_TICK: usize = 1;
+
+    fn make_raftstore_cfg(use_delete_range: bool) -> Arc<VersionTrack<Config>> {


all these configuration except use_delete_range parameter are hardcoded. So what's the point of creating a config instead of hardcode in the RegionRunner ?

Also please add a comment to clarify what's the consumer of this Config. It may be easily confused as the config used by raftstore itself.

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

…ngine-ext into merge-tikv-engine-trait

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

breezewish · 2022-09-08T12:28:00Z

/merge

ti-chi-bot · 2022-09-08T12:28:02Z

@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 /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.

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-09-08T12:28:06Z

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

Commit hash: 14b003a

ref tikv#12849 Support new engine trait can_apply_snapshot Signed-off-by: CalvinNeo <[email protected]>

CalvinNeo added 2 commits June 27, 2022 13:10

pub necessary interfaces

02b3492

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

introduce can_apply_snapshot

f31e1da

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

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. contribution This PR is from a community contributor. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 28, 2022

CalvinNeo mentioned this pull request Jun 28, 2022

Support TiKV using different KvEngines #12849

Closed

tonyxuqqi reviewed Jun 28, 2022

View reviewed changes

zhangjinpeng87 reviewed Jun 29, 2022

View reviewed changes

components/raftstore/src/store/worker/region.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Jun 29, 2022

View reviewed changes

components/raftstore/src/store/worker/region.rs Outdated Show resolved Hide resolved

CalvinNeo added 2 commits June 29, 2022 13:44

address comments part1

9959b3c

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

address comments 2

f721478

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

CalvinNeo requested a review from tonyxuqqi June 29, 2022 07:16

Merge branch 'master' into merge-tikv-engine-trait

3f450c6

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

m

d78b821

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

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

f

2814544

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

ti-chi-bot requested a review from BusyJay August 22, 2022 05:38

BusyJay reviewed Aug 22, 2022

View reviewed changes

f

1ef6248

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

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

region_worker_tick_interval and clean_stale_tick_max

81ae9ad

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

ti-chi-bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 24, 2022

CalvinNeo force-pushed the merge-tikv-engine-trait branch 3 times, most recently from 11d38f6 to 31ee9ca Compare September 6, 2022 12:24

Merge remote-tracking branch 'upstream/master' into merge-tikv-engine…

3d73188

…-trait

CalvinNeo force-pushed the merge-tikv-engine-trait branch from 31ee9ca to 3d73188 Compare September 6, 2022 14:20

ti-chi-bot requested a review from breezewish September 7, 2022 03:33

breezewish reviewed Sep 8, 2022

View reviewed changes

components/raftstore/src/store/config.rs Show resolved Hide resolved

CalvinNeo requested a review from tonyxuqqi September 8, 2022 02:27

Merge branch 'master' into merge-tikv-engine-trait

ea8a2b4

tonyxuqqi reviewed Sep 8, 2022

View reviewed changes

tonyxuqqi approved these changes Sep 8, 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 Sep 8, 2022

CalvinNeo and others added 7 commits September 8, 2022 12:57

f

4fd2b21

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

Merge branch 'merge-tikv-engine-trait' of github.com:CalvinNeo/tidb-e…

69f1969

…ngine-ext into merge-tikv-engine-trait

f

205b727

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

f

a54b05c

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

f

0475fee

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

f

ebd1497

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

Merge branch 'master' into merge-tikv-engine-trait

14b003a

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

ti-chi-bot merged commit 079a069 into tikv:master Sep 8, 2022

ti-chi-bot added this to the Pool milestone Sep 8, 2022

CalvinNeo added a commit to CalvinNeo/tidb-engine-ext that referenced this pull request Sep 9, 2022

raftstore: Implement engine trait can_apply_snapshot (tikv#12924)

53707d7

ref tikv#12849 Support new engine trait can_apply_snapshot Signed-off-by: CalvinNeo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: Implement engine trait can_apply_snapshot #12924

raftstore: Implement engine trait can_apply_snapshot #12924

CalvinNeo commented Jun 28, 2022

ti-chi-bot commented Jun 28, 2022 •

edited

Loading

CalvinNeo commented Jun 30, 2022

CalvinNeo commented Jun 30, 2022

CalvinNeo commented Jul 11, 2022

CalvinNeo commented Aug 22, 2022

CalvinNeo commented Aug 22, 2022

BusyJay Aug 22, 2022

BusyJay Aug 22, 2022

CalvinNeo Aug 22, 2022

BusyJay Aug 22, 2022 •

edited

Loading

CalvinNeo Aug 24, 2022 •

edited

Loading

CalvinNeo commented Sep 7, 2022

CalvinNeo commented Sep 7, 2022

breezewish commented Sep 8, 2022

tonyxuqqi Sep 8, 2022

tonyxuqqi Sep 8, 2022

breezewish commented Sep 8, 2022

ti-chi-bot commented Sep 8, 2022

ti-chi-bot commented Sep 8, 2022

raftstore: Implement engine trait can_apply_snapshot #12924

raftstore: Implement engine trait can_apply_snapshot #12924

Conversation

CalvinNeo commented Jun 28, 2022

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Jun 28, 2022 • edited Loading

CalvinNeo commented Jun 30, 2022

CalvinNeo commented Jun 30, 2022

CalvinNeo commented Jul 11, 2022

CalvinNeo commented Aug 22, 2022

CalvinNeo commented Aug 22, 2022

BusyJay Aug 22, 2022

Choose a reason for hiding this comment

BusyJay Aug 22, 2022

Choose a reason for hiding this comment

CalvinNeo Aug 22, 2022

Choose a reason for hiding this comment

BusyJay Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

CalvinNeo Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

CalvinNeo commented Sep 7, 2022

CalvinNeo commented Sep 7, 2022

breezewish commented Sep 8, 2022

tonyxuqqi Sep 8, 2022

Choose a reason for hiding this comment

tonyxuqqi Sep 8, 2022

Choose a reason for hiding this comment

breezewish commented Sep 8, 2022

ti-chi-bot commented Sep 8, 2022

ti-chi-bot commented Sep 8, 2022

ti-chi-bot commented Jun 28, 2022 •

edited

Loading

BusyJay Aug 22, 2022 •

edited

Loading

CalvinNeo Aug 24, 2022 •

edited

Loading