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

Refactor storage::kv to reduce abstraction duplication #9480

Open
brson opened this issue Jan 10, 2021 · 3 comments
Open

Refactor storage::kv to reduce abstraction duplication #9480

brson opened this issue Jan 10, 2021 · 3 comments
Labels
type/enhancement Type: Issue - Enhancement

Comments

@brson
Copy link
Contributor

brson commented Jan 10, 2021

This module contains its own traits that define the properties of a key-value store. These traits mostly duplicate engine_traits, while also introducing a few other capabilities, while also leaking details of rocksdb. I'm finding this module to be troublesome as I look for more opportunities to remove rocksdb-specifics from the tikv crate, and think it could use some cleanup.

I propose changing this module to mostly reuse the traits from engine_traits, adding new methods directly to engine_traits where it makes sense, and leaving other methods as extensions traits where those methods don't belong in engine_traits.

Specifically:

  • Rename all the *Ext traits in engine_traits to *EngineExt to indicate these are extensions to the main KvEngine trait. This step is needed to make room for new *Ext traits in the next step.
  • Rename kv::{Engine, Snapshot, Iterator} to KvEngineExt, SnapshotExt etc.
  • Make these new extension traits extend the traits in engine_traits, while deleting the duplicate methods.

There's definitely more cleanup that can be done from there (like putting btree_engine into its own crate), but these steps are about as far ahead as I can see offhand. I think getting this done would go a long way toward simplifying the kvengine abstractions within the tikv crate.

cc #6402

@brson brson added the type/enhancement Type: Issue - Enhancement label Jan 10, 2021
@brson
Copy link
Contributor Author

brson commented Jan 10, 2021

Some obvious followups:

  • Move btree_engine to its own crate, implementing engine_traits
  • Same for mock_engine
  • Delete the rocks_engine implementation inside tikv

Any kv engine abstractions within tikv should depend only on engine_traits.

@brson
Copy link
Contributor Author

brson commented Jan 10, 2021

It might be easier to start by moving btree_engine and mock_engine into their own crates.

@brson
Copy link
Contributor Author

brson commented Jan 14, 2021

This module uses very few other tikv modules, so once it is sufficiently cleaned up and minimized, it may be able to live in its own crate, maybe tikv_kv_store. Right now it doesn't look to me that it will be appropriate to completely merge this module into engine_traits, but it's hard to predict. I think this module can be cleaned up in many small ways.

ti-chi-bot pushed a commit that referenced this issue Jan 18, 2021
Signed-off-by: Brian Anderson <andersrb@gmail.com>

<!--
Thank you for contributing to TiKV!

If you haven't already, please read TiKV's [CONTRIBUTING](https://github.com/tikv/tikv/blob/master/CONTRIBUTING.md) document.

If you're unsure about anything, just ask; somebody should be along to answer within a day or two.

PR Title Format:
1. module [, module2, module3]: what's changed
2. *: what's changed

If you want to open the **Challenge Program** pull request, please use the following template:
https://raw.githubusercontent.com/tikv/.github/master/.github/PULL_REQUEST_TEMPLATE/challenge-program.md
You can use it with query parameters: https://github.com/tikv/tikv/compare/master...${you branch}?template=challenge-program.md
-->

### What problem does this PR solve?

Cleaning up the tikv::storage::kv module to make it eventually generic over engine_traits

- cc #9480
- cc #6402

### What is changed and how it works?

This code is unused. Delete it.

### Related changes


### Check List <!--REMOVE the items that are not applicable-->


### Release note <!-- bugfixes or new feature need a release note -->

- No release note.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Type: Issue - Enhancement
Projects
None yet
Development

No branches or pull requests

1 participant