multiget support for timestamps #6483
Conversation
hliu18
force-pushed
the
multiget-return-ts
branch
6 times, most recently
from
9d3cde0
to
bc61f9a
Compare
|
@riversand963 Can you please help review this PR? thanks. |
hliu18
force-pushed
the
multiget-return-ts
branch
7 times, most recently
from
86be63d
to
96213e9
Compare
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
cc @anand1976 since it is related to MultiGet. |
There was a problem hiding this comment.
LGTM. Thanks @hliu18 for the PR. Two more minor comments and we are good to go, unless @anand1976 has more comment.
|
There are some test failures that need to be investigated before landing. |
hliu18
force-pushed
the
multiget-return-ts
branch
from
4a7e3d1
to
b44913b
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
|
AppVeyor build failed due to timeout on VS 2017. It passed on VS 2017. Travis CI build failed with error "db_basic_test.cc:1705: undefined reference to `rocksdb::BlockBasedTable::kMultiGetReadStackBufSize'". It's not related to this change, and the code has been there since last year. It's not obvious to me why the build complains now. It's not on Windows. |
hliu18
force-pushed
the
multiget-return-ts
branch
from
b44913b
to
1bf8f63
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
hliu18
force-pushed
the
multiget-return-ts
branch
from
1bf8f63
to
aae842e
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@hliu18 I re-triggered the Travis build, but I am not sure about why some of them failed. Let's see if they occur again. |
hliu18
force-pushed
the
multiget-return-ts
branch
from
aae842e
to
b0815a3
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
hliu18
force-pushed
the
multiget-return-ts
branch
from
b0815a3
to
784b995
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/db_basic_test.cc
Outdated
| @@ -1701,8 +1702,8 @@ TEST_F(DBBasicTest, MultiGetIOBufferOverrun) { | |||
| BlockBasedTableOptions table_options; | |||
| table_options.pin_l0_filter_and_index_blocks_in_cache = true; | |||
| table_options.block_size = 16 * 1024; | |||
| assert(table_options.block_size > | |||
| BlockBasedTable::kMultiGetReadStackBufSize); | |||
| ASSERT_GT(table_options.block_size, | |||
There was a problem hiding this comment.
I think this change caused the failure. I am not sure why, but you can revert this change.
There was a problem hiding this comment.
Yes I found out it too. I'm going to use ASSERT_TRUE() instead.
There was a problem hiding this comment.
Please disregard my previous comment (deleted)
|
@hliu18 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
hliu18
force-pushed
the
multiget-return-ts
branch
from
5cffdd1
to
fe13b7b
Compare
|
@hliu18 has updated the pull request. Re-import the pull request |
|
I see it's checked in. Thanks. @riversand963 |
|
@riversand963 merged this pull request in a6ce5c8. |
Add timestamp support for MultiGet().
timestamp from readoptions is honored, and timestamps can be returned along with values.
MultiReadRandom perf test (10 minutes) on the same development machine ram drive with the same DB data shows no regression (within marge of error). The test is adapted from https://github.com/facebook/rocksdb/wiki/RocksDB-In-Memory-Workload-Performance-Benchmarks.
base line (commit 17bef7d):
multireadrandom : 104.173 micros/op 307167 ops/sec; (5462999 of 5462999 found)
This PR:
multireadrandom : 104.199 micros/op 307095 ops/sec; (5307999 of 5307999 found)
.\db_bench --db=r:\rocksdb.github --num_levels=6 --key_size=20 --prefix_size=20 --keys_per_prefix=0 --value_size=100 --cache_size=2147483648 --cache_numshardbits=6 --compression_type=none --compression_ratio=1 --min_level_to_compress=-1 --disable_seek_compaction=1 --hard_rate_limit=2 --write_buffer_size=134217728 --max_write_buffer_number=2 --level0_file_num_compaction_trigger=8 --target_file_size_base=134217728 --max_bytes_for_level_base=1073741824 --disable_wal=0 --wal_dir=r:\rocksdb.github\WAL_LOG --sync=0 --verify_checksum=1 --statistics=0 --stats_per_interval=0 --stats_interval=1048576 --histogram=0 --use_plain_table=1 --open_files=-1 --memtablerep=prefix_hash --bloom_bits=10 --bloom_locality=1 --duration=600 --benchmarks=multireadrandom --use_existing_db=1 --num=25000000 --threads=32 --allow_concurrent_memtable_write=0