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

Move SST code to engine traits #5790

Merged
merged 31 commits into from Nov 7, 2019
Merged

Move SST code to engine traits #5790

merged 31 commits into from Nov 7, 2019

Conversation

@brson
Copy link
Contributor

brson commented Nov 2, 2019

What have you changed?

This moves more SST code from engine to engine_traits/engine_rocks. After this PR all SST reading, writing, importing, and validation is done through the engine abstraction.

This doesn't yet remove the concrete rocks dependency from the sst_importer crate.

To support iteration over SstReader I had to use a reference-counted inner type instead of a reference. This is due to the unavailability of generic associated types. I expect to see more abstractions take reference counts as they are put behind associated types.

What is the type of the changes?

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

cargo check / test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

no

Does this PR affect tidb-ansible?

no

Refer to a related PR or issue link (optional)

#4184

Benchmark result if necessary (optional)

Any examples? (optional)

brson added 24 commits Oct 29, 2019
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
For consistency with existing types. Less confusing.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
For consistency with engine crate

Signed-off-by: Brian Anderson <andersrb@gmail.com>

wip
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson brson added the S: DNM label Nov 2, 2019
@brson brson requested review from kennytm, nrc, zhangjinpeng1987 and 5kbpers Nov 2, 2019
Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson brson force-pushed the brson:engine-traits-sst branch from f6d4e2a to afd744d Nov 2, 2019
snap: &DbSnapshot,
cf: CfName,
path: &str,
) -> Result<RocksSstWriter, Error> {

This comment has been minimized.

Copy link
@siddontang

siddontang Nov 3, 2019

Contributor

here we still use Rocks? will we return a SstWrtier interface later?

This comment has been minimized.

Copy link
@brson

brson Nov 4, 2019

Author Contributor

Yes, later the abstraction will be pushed further out.

@brson brson mentioned this pull request Nov 4, 2019
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 4, 2019

DNM now waiting to merge rust-rocksdb changes into tikv #5805

Signed-off-by: Brian Anderson <andersrb@gmail.com>
Copy link
Member

Connor1996 left a comment

LGTM overall

Cargo.toml Outdated
"raft-proto:0.6.0-alpha" = { git = "https://github.com/pingcap/raft-rs", branch = "master" }
"protobuf:2.8.0" = { git = "https://github.com/nrc/rust-protobuf", branch = "v2.8" }
"protobuf-codegen:2.8.0" = { git = "https://github.com/nrc/rust-protobuf", branch = "v2.8" }
#"raft:0.6.0-alpha" = { git = "https://github.com/pingcap/raft-rs", branch = "master" }

This comment has been minimized.

Copy link
@Connor1996

Connor1996 Nov 5, 2019

Member

should we clean it?

This comment has been minimized.

Copy link
@brson

brson Nov 6, 2019

Author Contributor

These comments are only temporary. I had to remove the [replace] section so I could use [patch] for the temporary git dependency replacement. I am going to revert both changes before merging, so this code will look the same as it did previously.

components/engine_traits/src/sst.rs Show resolved Hide resolved
Copy link
Contributor

nrc left a comment

Mostly looks good

components/test_sst_importer/src/lib.rs Show resolved Hide resolved
brson added 4 commits Nov 6, 2019
This reverts commit afd744d.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
This reverts commit d86c374.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
This reverts commit be532b0.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson brson force-pushed the brson:engine-traits-sst branch from dbd5373 to 3399a3b Nov 6, 2019
@brson brson removed the S: DNM label Nov 6, 2019
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 6, 2019

This is merged with master, the temporary rocksdb branch removed, and can be merged.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 6, 2019

Strange error on CI: looks like c++ crashed building rocksdb. It shouldn't be caused by this PR as it doesn't change the rocksdb dependency.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 6, 2019

/test

@nrc
nrc approved these changes Nov 6, 2019
Copy link
Contributor

nrc left a comment

lgtm

Copy link
Member

Connor1996 left a comment

LGTM

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

@brson merge failed.

@5kbpers

This comment has been minimized.

Copy link
Contributor

5kbpers commented Nov 7, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

@brson merge failed.

@5kbpers

This comment has been minimized.

Copy link
Contributor

5kbpers commented Nov 7, 2019

/test

@5kbpers 5kbpers merged commit b779f9c into tikv:master Nov 7, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Nov 7, 2019
* engine_*: outline SST traits

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: Use Rc inside RocksSstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: temporarily patch local rust-rocksdb

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: Implement SstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Add SstExt to KvEngine

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_importer: replace rocks SstReader with abstract SstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_*: Add SstWriter SstWriterBuilder

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: cleanup warnings

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_importer: Use generic SstWriter

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: add test_helpers mod and new_default_engine fn

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: port sst tests

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: remove sst module

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* raftstore: Use traits for SstWriter

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_service: Fix type annotations

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* raftstore: Use SST traits

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: temporarily use brson's rocksdb branch

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Rename CFOptions to ColumnFamilyOptions

For consistency with existing types. Less confusing.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Rename get_cf_handle to cf_handle

For consistency with engine crate

Signed-off-by: Brian Anderson <andersrb@gmail.com>

wip

* engine_rocks: Move sst test cases from engine to engine_rocks

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Introduce TitanDBOptions and re-enable titan sst test

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: cleanup unused

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* backup: use engine traits for SST

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: rustfmt

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: Move more sst functions into engine_rocks

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: cleanup unused

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: fix rocksdb patch

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: fix rocksdb patch"

This reverts commit afd744d.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: temporarily use brson's rocksdb branch"

This reverts commit d86c374.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: temporarily patch local rust-rocksdb"

This reverts commit be532b0.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
* engine_*: outline SST traits

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: Use Rc inside RocksSstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: temporarily patch local rust-rocksdb

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: Implement SstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Add SstExt to KvEngine

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_importer: replace rocks SstReader with abstract SstReader

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_*: Add SstWriter SstWriterBuilder

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: cleanup warnings

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_importer: Use generic SstWriter

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: add test_helpers mod and new_default_engine fn

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_rocks: port sst tests

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: remove sst module

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* raftstore: Use traits for SstWriter

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* sst_service: Fix type annotations

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* raftstore: Use SST traits

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: temporarily use brson's rocksdb branch

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Rename CFOptions to ColumnFamilyOptions

For consistency with existing types. Less confusing.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Rename get_cf_handle to cf_handle

For consistency with engine crate

Signed-off-by: Brian Anderson <andersrb@gmail.com>

wip

* engine_rocks: Move sst test cases from engine to engine_rocks

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine_traits: Introduce TitanDBOptions and re-enable titan sst test

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: cleanup unused

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* backup: use engine traits for SST

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: rustfmt

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: Move more sst functions into engine_rocks

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* engine: cleanup unused

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: fix rocksdb patch

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: fix rocksdb patch"

This reverts commit afd744d.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: temporarily use brson's rocksdb branch"

This reverts commit d86c374.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Revert "*: temporarily patch local rust-rocksdb"

This reverts commit be532b0.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.