From: Peter Dillinger <peterd@fb.com>
Date: Fri, 10 Jun 2022 02:43:19 +0000 (-0700)
Subject: Fix fragile CacheTest::ApplyToAllEntriesDuringResize (#10145)
X-Git-Tag: v7.4.3~51
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=5fa6ef7f1897b73732190c69f3ca4d3dc1795b25;p=rocksdb.git

Fix fragile CacheTest::ApplyToAllEntriesDuringResize (#10145)

Summary:
As seen in https://github.com/facebook/rocksdb/issues/10137, simply churning the cache key hashes (e.g.
by changing the raw cache keys) could trigger failure in this test, due
to possibility of some cache shard exceeding its portion of capacity
and evicting entries. Updated the test to be less fragile by using
greater margins, and added a pre-check for evictions, which doesn't
manifest as a race condition, before the main check that can race.

Also added stack trace handler to cache_test for debugging.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10145

Test Plan:
test thousands of iterations with gtest-parallel, including
with changes in https://github.com/facebook/rocksdb/issues/10137 that were surfacing the problem. Pre-check
without the fix would always fail with https://github.com/facebook/rocksdb/issues/10137

Reviewed By: guidotag

Differential Revision: D37058771

Pulled By: pdillinger

fbshipit-source-id: a7cf137967aef49c07ae9602d8523c63e7388fab
---

diff --git a/cache/cache_test.cc b/cache/cache_test.cc
index 735f77744..05bdc0056 100644
--- a/cache/cache_test.cc
+++ b/cache/cache_test.cc
@@ -18,6 +18,7 @@
 #include "cache/clock_cache.h"
 #include "cache/fast_lru_cache.h"
 #include "cache/lru_cache.h"
+#include "port/stack_trace.h"
 #include "test_util/testharness.h"
 #include "util/coding.h"
 #include "util/string_util.h"
@@ -788,8 +789,10 @@ TEST_P(CacheTest, ApplyToAllEntriesDuringResize) {
   constexpr int kSpecialCharge = 2;
   constexpr int kNotSpecialCharge = 1;
   constexpr int kSpecialCount = 100;
+  size_t expected_usage = 0;
   for (int i = 0; i < kSpecialCount; ++i) {
     Insert(i, i * 2, kSpecialCharge);
+    expected_usage += kSpecialCharge;
   }
 
   // For callback
@@ -810,12 +813,17 @@ TEST_P(CacheTest, ApplyToAllEntriesDuringResize) {
   });
 
   // In parallel, add more entries, enough to cause resize but not enough
-  // to cause ejections
-  for (int i = kSpecialCount * 1; i < kSpecialCount * 6; ++i) {
+  // to cause ejections. (Note: if any cache shard is over capacity, there
+  // will be ejections)
+  for (int i = kSpecialCount * 1; i < kSpecialCount * 5; ++i) {
     Insert(i, i * 2, kNotSpecialCharge);
+    expected_usage += kNotSpecialCharge;
   }
 
   apply_thread.join();
+  // verify no evictions
+  ASSERT_EQ(cache_->GetUsage(), expected_usage);
+  // verify everything seen in ApplyToAllEntries
   ASSERT_EQ(special_count, kSpecialCount);
 }
 
@@ -859,6 +867,7 @@ INSTANTIATE_TEST_CASE_P(CacheTestInstance, LRUCacheTest,
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {
+  ROCKSDB_NAMESPACE::port::InstallStackTraceHandler();
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }