Fix Get() return status when block cache is disabled #8485

hongrubb · 2021-07-06T05:08:44Z

This PR is for #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.

table/block_based/block_based_table_reader.cc

facebook-github-bot · 2021-07-08T01:33:05Z

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr

Thanks for finding/fixing the problem! This feels like something that's reasonably straightforward to cover in a test case, if you have time.

ajkr

Also do you mind updating HISTORY.md with a "Bug Fixes" item for preventing empty value returned for non-existent key, and a separate item for fixing dropped stat block cache counter updates for failed queries?

ajkr · 2021-07-08T02:38:05Z

db/version_set.cc

@@ -1938,16 +1938,16 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k,
      PERF_COUNTER_BY_LEVEL_ADD(get_from_table_nanos, timer.ElapsedNanos(),
                                fp.GetHitFileLevel());
    }
-    if (!status->ok()) {


Actually is it better to call ReportCounters() in this old !status->ok() conditional body? Seems like they are supposed to be updated before return as long as db_statistics_ != nullptr.

I don't know if I understand you correctly.
You mean the code should look like this?

if (!status->ok()) { if (db_statistics_ != nullptr) { get_context.ReportCounters(); } return; }

If that, I will change the code soon.

hongrubb · 2021-07-08T05:34:02Z

Thanks for finding/fixing the problem! This feels like something that's reasonably straightforward to cover in a test case, if you have time.

Sorry, I don't have time to add test cases right now. But I'd like to contribute test cases in later weeks. Is it ok to submit another PR for test cases?

hongrubb · 2021-07-08T05:47:00Z

Also do you mind updating HISTORY.md with a "Bug Fixes" item for preventing empty value returned for non-existent key, and a separate item for fixing dropped stat block cache counter updates for failed queries?

I'd like to do that, I will update it soon.

ajkr · 2021-07-08T16:51:17Z

Thanks for finding/fixing the problem! This feels like something that's reasonably straightforward to cover in a test case, if you have time.

Sorry, I don't have time to add test cases right now. But I'd like to contribute test cases in later weeks. Is it ok to submit another PR for test cases?

Sure, sounds good.

facebook-github-bot · 2021-07-10T10:04:47Z

@hongrubb has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-07-10T10:08:18Z

@hongrubb has updated the pull request. You must reimport the pull request before landing.

ajkr

Looks great, thank you very much!

facebook-github-bot · 2021-07-12T19:19:50Z

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-07-12T20:53:36Z

@hongrubb has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-07-12T20:53:48Z

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr · 2021-07-12T23:28:15Z

Do you mind checking this test?

stdout: Note: Google Test filter = DBBlockCacheTest.TestWithCompressedBlockCache
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from DBBlockCacheTest
[ RUN      ] DBBlockCacheTest.TestWithCompressedBlockCache
internal_repo_rocksdb/repo/db/db_block_cache_test.cc:99: Failure
Expected equality of these values:
  miss_count_ + expected_misses
    Which is: 9
  new_miss_count
    Which is: 10
internal_repo_rocksdb/repo/db/db_block_cache_test.cc:99: Failure
Expected equality of these values:
  miss_count_ + expected_misses
    Which is: 10
  new_miss_count

This is from our internal pre-land test run; I don't see the error in any test run here somehow...

hongrubb · 2021-07-12T23:46:00Z

Do you mind checking this test?

stdout: Note: Google Test filter = DBBlockCacheTest.TestWithCompressedBlockCache
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from DBBlockCacheTest
[ RUN      ] DBBlockCacheTest.TestWithCompressedBlockCache
internal_repo_rocksdb/repo/db/db_block_cache_test.cc:99: Failure
Expected equality of these values:
  miss_count_ + expected_misses
    Which is: 9
  new_miss_count
    Which is: 10
internal_repo_rocksdb/repo/db/db_block_cache_test.cc:99: Failure
Expected equality of these values:
  miss_count_ + expected_misses
    Which is: 10
  new_miss_count

This is from our internal pre-land test run; I don't see the error in any test run here somehow...

Sure.

hongrubb · 2021-07-13T11:23:11Z

@ajkr Hi, I think I have found the problem. It's very interesting!

I found the failed test was added by #8242 25days ago.
The test failed at CheckCacheCounters(), but I think we should update the counter even though the Get() is failed.

  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);

@akankshamahajan15 thanks for the test and detailed comment in code, it helped me a lot. And please take a look at this problem, maybe we should change this test.

akankshamahajan15 · 2021-07-13T20:37:55Z

The test failed at CheckCacheCounters(), but I think we should update the counter even though the Get() is failed.
  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);
@akankshamahajan15 thanks for the test and detailed comment in code, it helped me a lot. And please take a look at this problem, maybe we should change this test.

Since you fixed the bug of adding statistics when query is failed", this test should be updated to take that into account. Please feel free to update the test. Thanks for fixing the bug.

facebook-github-bot · 2021-07-13T22:24:04Z

@hongrubb has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-07-13T22:27:14Z

@hongrubb has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-07-14T00:22:40Z

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-07-14T01:13:38Z

@ajkr merged this pull request in 8700332.

Fix Get() return status when block cache is disabled

4069806

facebook-github-bot added the CLA Signed label Jul 6, 2021

hongrubb mentioned this pull request Jul 6, 2021

Get with kBlockCacheTier return Status::OK() when block cache is disabled #8453

Open

isaac-io reviewed Jul 6, 2021

View reviewed changes

table/block_based/block_based_table_reader.cc Show resolved Hide resolved

ajkr approved these changes Jul 8, 2021

View reviewed changes

ajkr reviewed Jul 8, 2021

View reviewed changes

update HISTORY.md and fix code

6a24c72

hongrubb force-pushed the master branch from ada8e17 to 6a24c72 Compare July 10, 2021 10:08

ajkr approved these changes Jul 12, 2021

View reviewed changes

Merge branch 'master' into master

a30cff1

fix test case in db_block_cache_test.cc

a6a54c7

hongrubb force-pushed the master branch from 6b6a05c to a6a54c7 Compare July 13, 2021 22:27

facebook-github-bot closed this in 8700332 Jul 14, 2021

facebook-github-bot added the Merged label Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Get() return status when block cache is disabled #8485

Fix Get() return status when block cache is disabled #8485

hongrubb commented Jul 6, 2021

facebook-github-bot commented Jul 8, 2021

ajkr left a comment

ajkr left a comment

ajkr Jul 8, 2021

hongrubb Jul 8, 2021

ajkr Jul 8, 2021

hongrubb commented Jul 8, 2021

hongrubb commented Jul 8, 2021

ajkr commented Jul 8, 2021

facebook-github-bot commented Jul 10, 2021

facebook-github-bot commented Jul 10, 2021

ajkr left a comment

facebook-github-bot commented Jul 12, 2021

facebook-github-bot commented Jul 12, 2021

facebook-github-bot commented Jul 12, 2021

ajkr commented Jul 12, 2021

hongrubb commented Jul 12, 2021

hongrubb commented Jul 13, 2021 •

edited

Loading

akankshamahajan15 commented Jul 13, 2021

facebook-github-bot commented Jul 13, 2021

facebook-github-bot commented Jul 13, 2021

facebook-github-bot commented Jul 14, 2021

facebook-github-bot commented Jul 14, 2021

Fix Get() return status when block cache is disabled #8485

Fix Get() return status when block cache is disabled #8485

Conversation

hongrubb commented Jul 6, 2021

facebook-github-bot commented Jul 8, 2021

ajkr left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

ajkr Jul 8, 2021

Choose a reason for hiding this comment

hongrubb Jul 8, 2021

Choose a reason for hiding this comment

ajkr Jul 8, 2021

Choose a reason for hiding this comment

hongrubb commented Jul 8, 2021

hongrubb commented Jul 8, 2021

ajkr commented Jul 8, 2021

facebook-github-bot commented Jul 10, 2021

facebook-github-bot commented Jul 10, 2021

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 12, 2021

facebook-github-bot commented Jul 12, 2021

facebook-github-bot commented Jul 12, 2021

ajkr commented Jul 12, 2021

hongrubb commented Jul 12, 2021

hongrubb commented Jul 13, 2021 • edited Loading

akankshamahajan15 commented Jul 13, 2021

facebook-github-bot commented Jul 13, 2021

facebook-github-bot commented Jul 13, 2021

facebook-github-bot commented Jul 14, 2021

facebook-github-bot commented Jul 14, 2021

hongrubb commented Jul 13, 2021 •

edited

Loading