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

kv/RocksDBStore: implement rm_range_keys operator interface and test #13855

Merged
merged 2 commits into from Mar 28, 2017

Conversation

Projects
None yet
2 participants
@yuyuyu101
Member

yuyuyu101 commented Mar 8, 2017

Signed-off-by: Haomai Wang haomai@xsky.com

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Mar 8, 2017

Member

replacing #11198

Member

yuyuyu101 commented Mar 8, 2017

replacing #11198

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 8, 2017

Member

// Removes the database entries in the range ["begin_key", "end_key"), i.e.,
// including "begin_key" and excluding "end_key". Returns OK on success, and
// a non-OK status on error. It is not an error if no keys exist in the range
// ["begin_key", "end_key").
//
// This feature is currently an experimental performance optimization for
// deleting very large ranges of contiguous keys. Invoking it many times or on
// small ranges may severely degrade read performance; in particular, the
// resulting performance can be worse than calling Delete() for each key in
// the range. Note also the degraded read performance affects keys outside the
// deleted ranges, and affects database operations involving scans, like flush
// and compaction.
//
// Consider setting ReadOptions::ignore_range_deletions = true to speed
// up reads for key(s) that are known to be unaffected by range deletions.

This scared me away before. It would be great, though, to avoid the current flush() we have to do in BlueStore to support this today (to ensure recent new keys are applied so that they get deleted). Any sense of whether this warning is as scary as it sounds?

(Also, rebase?)

Member

liewegas commented Mar 8, 2017

// Removes the database entries in the range ["begin_key", "end_key"), i.e.,
// including "begin_key" and excluding "end_key". Returns OK on success, and
// a non-OK status on error. It is not an error if no keys exist in the range
// ["begin_key", "end_key").
//
// This feature is currently an experimental performance optimization for
// deleting very large ranges of contiguous keys. Invoking it many times or on
// small ranges may severely degrade read performance; in particular, the
// resulting performance can be worse than calling Delete() for each key in
// the range. Note also the degraded read performance affects keys outside the
// deleted ranges, and affects database operations involving scans, like flush
// and compaction.
//
// Consider setting ReadOptions::ignore_range_deletions = true to speed
// up reads for key(s) that are known to be unaffected by range deletions.

This scared me away before. It would be great, though, to avoid the current flush() we have to do in BlueStore to support this today (to ensure recent new keys are applied so that they get deleted). Any sense of whether this warning is as scary as it sounds?

(Also, rebase?)

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Mar 8, 2017

Member

ok, let's pending this PR. because I see active contributes on this area in rocksdb upstream. compared before, this api just can work.

Member

yuyuyu101 commented Mar 8, 2017

ok, let's pending this PR. because I see active contributes on this area in rocksdb upstream. compared before, this api just can work.

@yuyuyu101 yuyuyu101 changed the title from kv/RocksDBStore: implement rm_range_keys operator interface and test to DNM: kv/RocksDBStore: implement rm_range_keys operator interface and test Mar 8, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 8, 2017

Member
Member

liewegas commented Mar 8, 2017

@yuyuyu101 yuyuyu101 changed the title from DNM: kv/RocksDBStore: implement rm_range_keys operator interface and test to kv/RocksDBStore: implement rm_range_keys operator interface and test Mar 10, 2017

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Mar 13, 2017

Member

retest this please

Member

yuyuyu101 commented Mar 13, 2017

retest this please

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 24, 2017

Member

2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/0, where GetParam() = "leveldb"
2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/1, where GetParam() = "rocksdb"
2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/2, where GetParam() = "memdb"

Member

liewegas commented Mar 24, 2017

2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/0, where GetParam() = "leveldb"
2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/1, where GetParam() = "rocksdb"
2017-03-23T23:35:50.279 INFO:teuthology.orchestra.run.smithi028.stdout:[ FAILED ] KeyValueDB/KVTest.RMRange/2, where GetParam() = "memdb"

yuyuyu101 added some commits Sep 21, 2016

kv/RocksDBStore: implement rm_range_keys operator interface and test
Signed-off-by: Haomai Wang <haomai@xsky.com>
kv/rocksdb: add option to control whether enable DeleteRange API
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Mar 26, 2017

Member

[----------] Global test environment tear-down
[==========] 21 tests from 1 test case ran. (3637 ms total)
[ PASSED ] 21 tests.

Member

yuyuyu101 commented Mar 26, 2017

[----------] Global test environment tear-down
[==========] 21 tests from 1 test case ran. (3637 ms total)
[ PASSED ] 21 tests.

@yuyuyu101

This comment has been minimized.

Show comment
Hide comment
@yuyuyu101

yuyuyu101 Mar 26, 2017

Member

fixed!

Member

yuyuyu101 commented Mar 26, 2017

fixed!

@yuyuyu101 yuyuyu101 added the needs-qa label Mar 26, 2017

@liewegas liewegas merged commit ba97f1f into ceph:master Mar 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@yuyuyu101 yuyuyu101 deleted the yuyuyu101:wip-bluestore-rm-range branch Mar 28, 2017

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