From 870033291ab00447dfebdba29361df4ca13799cc Mon Sep 17 00:00:00 2001 From: hongrubb Date: Tue, 13 Jul 2021 18:12:03 -0700 Subject: [PATCH] Fix Get() return status when block cache is disabled (#8485) Summary: This PR is for https://github.com/facebook/rocksdb/issues/8453 We need to update `s = biter.status();` when `biter.status().IsIncomplete()` is true. By doing this, can fix the problem in issue. Besides, we still need to update `db_statistics` in `get_context.ReportCounters()` before return back. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8485 Reviewed By: jay-zhuang Differential Revision: D29604835 Pulled By: ajkr fbshipit-source-id: c7f2f1cd058223ce1b507ec05d57cf264b9c9710 --- HISTORY.md | 2 ++ db/db_block_cache_test.cc | 4 ++-- db/version_set.cc | 3 +++ table/block_based/block_based_table_reader.cc | 1 + 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 07d7425b9..b3309a3c1 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,6 +8,8 @@ * `GetLiveFilesMetaData()` now populates the `temperature`, `oldest_ancester_time`, and `file_creation_time` fields of its `LiveFileMetaData` results when the information is available. Previously these fields always contained zero indicating unknown. * Fix mismatches of OnCompaction{Begin,Completed} in case of DisableManualCompaction(). * Fix continuous logging of an existing background error on every user write +* Fix a bug that `Get()` return Status::OK() and an empty value for non-existent key when `read_options.read_tier = kBlockCacheTier`. +* Fix a bug that stat in `get_context` didn't accumulate to statistics when query is failed. ### New Features * ldb has a new feature, `list_live_files_metadata`, that shows the live SST files, as well as their LSM storage level and the column family they belong to. diff --git a/db/db_block_cache_test.cc b/db/db_block_cache_test.cc index 6108bf5b7..92d2e7419 100644 --- a/db/db_block_cache_test.cc +++ b/db/db_block_cache_test.cc @@ -299,8 +299,8 @@ TEST_F(DBBlockCacheTest, TestWithCompressedBlockCache) { // Load last key block. ASSERT_EQ("Result incomplete: Insert failed due to LRU cache being full.", Get(ToString(kNumBlocks - 1))); - // Failure won't record the miss counter. - CheckCacheCounters(options, 0, 0, 0, 1); + // Failure will also record the miss counter. + CheckCacheCounters(options, 1, 0, 0, 1); CheckCompressedCacheCounters(options, 1, 0, 1, 0); // Clear strict capacity limit flag. This time we shall hit compressed block diff --git a/db/version_set.cc b/db/version_set.cc index 660b46985..e80bf67d6 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1939,6 +1939,9 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, fp.GetHitFileLevel()); } if (!status->ok()) { + if (db_statistics_ != nullptr) { + get_context.ReportCounters(); + } return; } diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index acb58138d..d5b1d5e83 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -2341,6 +2341,7 @@ Status BlockBasedTable::Get(const ReadOptions& read_options, const Slice& key, // Update Saver.state to Found because we are only looking for // whether we can guarantee the key is not there when "no_io" is set get_context->MarkKeyMayExist(); + s = biter.status(); break; } if (!biter.status().ok()) {