Configurable delete range #2573
Conversation
…ingcap/tikv into zhangjinpeng/configurable-delete-range
…87/tikv into configurable-delete-range
zhangjinpeng87
changed the title
|
PTAL |
src/util/rocksdb/mod.rs
Outdated
| @@ -151,8 +148,7 @@ fn check_and_open( | |||
| } | |||
| } | |||
| } | |||
| let cfds = cfs_v.into_iter().zip(cfs_opts_v).collect(); | |||
| let mut db = DB::open_cf(db_opt, path, cfds).unwrap(); | |||
| let mut db = DB::open_cf(db_opt, path, cfs_v.into_iter().zip(cfs_opts_v).collect()).unwrap(); | |||
There was a problem hiding this comment.
Why combine these into one line?
src/raftstore/store/worker/apply.rs
Outdated
| @@ -309,6 +309,8 @@ pub struct ApplyDelegate { | |||
| term: u64, | |||
| pending_cmds: PendingCmdQueue, | |||
| metrics: ApplyMetrics, | |||
|
|
|||
| use_delete_range: bool, | |||
There was a problem hiding this comment.
Is it more suitable to put this to ApplyContext?
…87/tikv into configurable-delete-range
src/raftstore/store/worker/apply.rs
Outdated
| @@ -1974,7 +1996,7 @@ mod tests { | |||
| let obs = ApplyObserver::default(); | |||
| host.registry | |||
| .register_query_observer(1, Box::new(obs.clone())); | |||
| let mut apply_ctx = ApplyContext::new(&host); | |||
| let mut apply_ctx = ApplyContext::new(&host, true /* use delete range */); | |||
|
Do we still need to support the test without DeleteRange now? Rest LGTM PTAL @huachaohuang @BusyJay |
|
LGTM |
|
@zhangjinpeng1987 I think we can merge this, and use delete range as default in our all tests now. |
|
/run-all-tests |
| @@ -254,7 +260,9 @@ impl SnapContext { | |||
| escape(&start_key), | |||
| escape(&end_key) | |||
| ); | |||
| if let Err(e) = util::delete_all_in_range(&self.kv_db, &start_key, &end_key) { | |||
| if let Err(e) = | |||
| util::delete_all_in_range(&self.kv_db, &start_key, &end_key, self.use_delete_range) | |||
There was a problem hiding this comment.
I don't think it's appropriate to use delete range here. For long running queries, delete all sst files directly may introduce wrong result.
There was a problem hiding this comment.
delete range will not break snapshot, so it is ok.
There was a problem hiding this comment.
Why? When SST files are deleted, keys in the bottom level can be seen by the snapshot.
There was a problem hiding this comment.
delete range never drop sst files directly.
There was a problem hiding this comment.
Are you misunderstand delete files in range and delete range.
There was a problem hiding this comment.
Oh, I see. I thought use_delete_range is a hint for delete_file_in_range. It's OK to use delete_range here. Thanks for the explanation.
No description provided.