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

In-memory Engine: enable HybridEngine only for storage and coprocessor modules #17181

Closed
overvenus opened this issue Jun 24, 2024 · 0 comments · Fixed by #17359
Closed

In-memory Engine: enable HybridEngine only for storage and coprocessor modules #17181

overvenus opened this issue Jun 24, 2024 · 0 comments · Fixed by #17359
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@overvenus
Copy link
Member

Background:

HybridEngine wraps RocksDB and region_cache_memory_engine, providing a unified interface through the engine_traits. Currently, to enable the in-memory engine feature, TiKV has to replace RocksDB with HybridEngine for all components, which could cause compatibility issues.

Problem:

Replacing RocksDB with HybridEngine for all components can lead to potential compatibility problems and increases the testing burden.

Proposal:

Since Storage and Coprocessor are the only components serving write and read requests, we should only enable HybridEngine for these components. This approach will reduce compatibility testing burdens for unrelated components.
@overvenus overvenus added the type/enhancement The issue or PR belongs to an enhancement. label Jun 24, 2024
@overvenus overvenus mentioned this issue Jun 24, 2024
Open
70 tasks
@overvenus overvenus mentioned this issue Aug 5, 2024
Merged
1 task
This was referenced Aug 13, 2024
Merged
Merged
overvenus added a commit that referenced this issue Sep 6, 2024
@overvenus
In-memory Engine: Reduce HybridEngine scope (#17359)
7e8d4ad 
In TiKV, the `KvEngine` trait provides the write path abstraction
(`WriteBatch`) and read path abstraction (`Snapshot`). The
`HybridEngine` implements the `KvEngine` trait, allowing it to be
integrated into TiKV.

However, as described in issue #17181, there is a major downside: all
components and features in TiKV use the `HybridEngine`, which can
introduce unexpected compatibility issues (e.g., BR restore failures
with checksum errors). This is not worthwhile because the `HybridEngine`
is designed specifically for accelerating coprocessor read speed in
scenarios with many MVCC workloads.

This PR shrinks the `HybridEngine` scope from being globally enabled to
only being enabled in the coprocessor. The changes include:
- Adding a new observer, `WriteBatchObserver`, to observe write requests
  via raftstore coprocessor hooks.
- Adding a new observer, `SnapshotObserver`, to observe snapshot (and
  `read_ts`) via raftstore coprocessor hooks.
- Adding a new method to the `RaftKv` struct called `async_in_memory_snapshot`,
  which takes snapshots from the `HybridEngine`.

This PR also introduces an architectural change, transforming the
in-memory engine from functioning as an `KvEngine` to acting as an
MVCC-aware cache. Conceptually, the in-memory engine is shifted from
the storage engine layer to the MVCC layer. This adjustment better
aligns with its purpose of improving MVCC performance by caching recent
versions, rather than serving as a full-fledged storage engine.

    ┌──────────────┐      ┌──────────────┐
    │ Copr/Storage │      │ Copr/Storage │
    ├──────────────┤      ├──────────────┤
    │    MVCC      │      │     MVCC     │
    ├──────────────┤      │ ┌──────────┐ │
    │ Raftstore    │ ───> │ │   IME    │ │
    ├──────────────┤      ├─┴──────────┴─┤
    │    IME       │      │  Raftstore   │
    │┌────────────┐│      ├──────────────┤
    ││  RocksDB   ││      │    RocksDB   │
    └┴────────────┴┘      └──────────────┘

Additionally, this PR brings two unexpected benefits*:
- Reduced TiKV binary size: From 1.7GB to 1.2GB.
- Faster build time: From 32 minutes to 20 minutes.

*: Results were obtained using the `make release` command.

Signed-off-by: Neil Shen <overvenus@gmail.com>
ti-chi-bot bot added a commit that referenced this issue Sep 20, 2024
@overvenus @ti-chi-bot
In-memory Engine: decoupling raftstore and IME (#17386)
29e4910 
ref #17181

* Do not disable snapshot cache when IME is enabled

    Since `async_in_memory_snapshot` always takes an in-memory engine
    snapshot regardless of whether the RocksDB snapshot is cached, we can
    safely revert #16863 and address @tonyxuqqi's concern raised in
    #16863 (comment)

* Remove SnapshotContext from LocalReadRouter

    This commit partially reverts #16181.

* Remove SnapshotContext from KvEngine::snapshot

    This commit partially reverts #16163. By removing `SnapshotContext`
    from `KvEngine::snapshot`, raftstore is no longer coupled with the
    In-Memory Engine.

Signed-off-by: Neil Shen <overvenus@gmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ti-chi-bot bot added a commit that referenced this issue Oct 29, 2024
@overvenus @ti-chi-bot
In-memory Engine: Remove boilerplate code (#17380)
31a2160 
ref #17181

This commit is a follow-up of #17359. Since `HybridEngine` is now
integrated into TiKV through coprocessor observers and RaftKv, it no
longer needs to implement the Engine traits. This commit removes the
unnecessary boilerplate code for `HybridEngine`.

Signed-off-by: Neil Shen <overvenus@gmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@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

Successfully merging a pull request may close this issue.

In-memory Engine: Reduce HybridEngine scope overvenus/tikv
1 participant
@overvenus