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

raft cf use dedicated rocksdb #2054

Merged
merged 65 commits into from Aug 22, 2017

Conversation

Projects
None yet
4 participants
@lishuai87
Contributor

lishuai87 commented Jul 19, 2017

No description provided.

Show outdated Hide outdated etc/config-template.toml Outdated
@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Jul 29, 2017

Contributor

Is this pr ready to be merged?

Contributor

BusyJay commented Jul 29, 2017

Is this pr ready to be merged?

@lishuai87 lishuai87 changed the base branch from pre-ga to master Aug 3, 2017

Show outdated Hide outdated etc/config-template.toml Outdated
Show outdated Hide outdated etc/config-template.toml Outdated
Show outdated Hide outdated src/bin/tikv-ctl.rs Outdated
Show outdated Hide outdated src/bin/tikv-server.rs Outdated
Show outdated Hide outdated src/config.rs Outdated
Show outdated Hide outdated src/raftstore/coprocessor/region_snapshot.rs Outdated
Show outdated Hide outdated tests/storage/test_raftkv.rs Outdated
Show outdated Hide outdated tests/raftstore/test_compact_after_delete.rs Outdated
@@ -245,7 +252,7 @@ impl Simulator for ServerCluster {
}
pub fn new_server_cluster(id: u64, count: usize) -> Cluster<ServerCluster> {
new_server_cluster_with_cfs(id, count, ALL_CFS)
new_server_cluster_with_cfs(id, count, &[])

This comment has been minimized.

@BusyJay

BusyJay Aug 18, 2017

Contributor

Why remove these?

@BusyJay

BusyJay Aug 18, 2017

Contributor

Why remove these?

This comment has been minimized.

@lishuai87

lishuai87 Aug 19, 2017

Contributor

because Cluster create_engines will create ALL_CFS for kv, CF_DEFAULT for raft by default.

@lishuai87

lishuai87 Aug 19, 2017

Contributor

because Cluster create_engines will create ALL_CFS for kv, CF_DEFAULT for raft by default.

Show outdated Hide outdated tests/raftstore/server.rs Outdated
Show outdated Hide outdated src/util/rocksdb/metrics_flusher.rs Outdated
Show outdated Hide outdated src/util/rocksdb/engine_metrics.rs Outdated
let mut ident = StoreIdent::new();
if !try!(is_range_empty(
engine,
&engines.kv_engine,

This comment has been minimized.

@BusyJay

BusyJay Aug 18, 2017

Contributor

Should check raft_engine too.

@BusyJay

BusyJay Aug 18, 2017

Contributor

Should check raft_engine too.

Show outdated Hide outdated src/raftstore/store/bootstrap.rs Outdated

lishuai87 added some commits Aug 18, 2017

Show outdated Hide outdated src/bin/tikv-server.rs Outdated
@BusyJay

We need to prevent user from upgrading from old version to new version accidentally, which may drop raft cf and can't rollback to old version any more.

Show outdated Hide outdated src/raftstore/store/store.rs Outdated
Show outdated Hide outdated src/util/rocksdb/mod.rs Outdated
@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Aug 20, 2017

Contributor

it is better to record a version key (1.0.0) in KV RocksDB, if not exist, we can not start. And I think it is dangerous to drop the discarded CF directly.

Btw, I think we should keep raft CF in KV RocksDB and still save raft apply index, if not, each apply will cover the whole range from 0x01 (apply index) to 'z' (data), which may cause compaction high pressure. /cc @zhangjinpeng1987

Contributor

siddontang commented Aug 20, 2017

it is better to record a version key (1.0.0) in KV RocksDB, if not exist, we can not start. And I think it is dangerous to drop the discarded CF directly.

Btw, I think we should keep raft CF in KV RocksDB and still save raft apply index, if not, each apply will cover the whole range from 0x01 (apply index) to 'z' (data), which may cause compaction high pressure. /cc @zhangjinpeng1987

Show outdated Hide outdated src/bin/tikv-ctl.rs Outdated
Show outdated Hide outdated src/config.rs Outdated
Show outdated Hide outdated src/config.rs Outdated
@siddontang

Rest LGTM

@lishuai87 lishuai87 merged commit a08ed0d into master Aug 22, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@choleraehyq choleraehyq deleted the javaforfun/apply-state-in-kv-rocksdb branch Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment