This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new ffc1b11f6 KUDU-613: Fix minor metrics related bug
ffc1b11f6 is described below
commit ffc1b11f6b54f0e799146f2e444c5b001827dd31
Author: Mahesh Reddy <[email protected]>
AuthorDate: Fri Oct 18 14:02:01 2024 -0400
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Reviewed-on: http://gerrit.cloudera.org:8080/21956
Tested-by: Alexey Serbin <[email protected]>
Reviewed-by: Alexey Serbin <[email protected]>
---
src/kudu/util/slru_cache-test.cc | 52 +++++++++++++++++++++++++++++++++++++++-
src/kudu/util/slru_cache.cc | 1 +
src/kudu/util/slru_cache.h | 3 +++
3 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/kudu/util/slru_cache-test.cc b/src/kudu/util/slru_cache-test.cc
index 4b05befc7..68b1547d1 100644
--- a/src/kudu/util/slru_cache-test.cc
+++ b/src/kudu/util/slru_cache-test.cc
@@ -296,8 +296,11 @@ TEST_P(SLRUCacheTest, Erase) {
ASSERT_EQ(201, evicted_values_[1]);
}
-// Underlying entry isn't actually deleted until handle around it from lookup
is reset.
+// Underlying entry isn't actually freed until handle around it from lookup is
reset.
TEST_P(SLRUCacheTest, EntriesArePinned) {
+ auto* metrics(dynamic_cast<SLRUCacheMetrics*>(slru_cache_->metrics_.get()));
+ ASSERT_TRUE(metrics);
+
Insert(100, 101);
auto h1 = slru_cache_->Lookup(EncodeInt(100), Cache::EXPECT_IN_CACHE);
ASSERT_EQ(101, DecodeInt(slru_cache_->Value(h1)));
@@ -306,20 +309,67 @@ TEST_P(SLRUCacheTest, EntriesArePinned) {
auto h2 = slru_cache_->Lookup(EncodeInt(100), Cache::EXPECT_IN_CACHE);
ASSERT_EQ(102, DecodeInt(slru_cache_->Value(h2)));
ASSERT_EQ(0, evicted_keys_.size());
+ ASSERT_EQ(0, metrics->probationary_segment_evictions.get()->value());
+ // Reset lookup handle of upserted entry, entry is now freed.
h1.reset();
ASSERT_EQ(1, evicted_keys_.size());
ASSERT_EQ(100, evicted_keys_[0]);
ASSERT_EQ(101, evicted_values_[0]);
+ ASSERT_EQ(1, metrics->probationary_segment_evictions.get()->value());
+ // Upserted entry is erased, but lookup handle still exists so entry is not
freed.
Erase(100);
ASSERT_EQ(-1, Lookup(100));
ASSERT_EQ(1, evicted_keys_.size());
+ ASSERT_EQ(1, metrics->probationary_segment_evictions.get()->value());
+ // Reset lookup handle, entry is now freed.
h2.reset();
ASSERT_EQ(2, evicted_keys_.size());
ASSERT_EQ(100, evicted_keys_[1]);
ASSERT_EQ(102, evicted_values_[1]);
+ ASSERT_EQ(2, metrics->probationary_segment_evictions.get()->value());
+
+ Insert(200, 201);
+ auto h3 = slru_cache_->Lookup(EncodeInt(200), Cache::EXPECT_IN_CACHE);
+ ASSERT_EQ(201, DecodeInt(slru_cache_->Value(h3)));
+ // Upgrade entry to protected segment.
+ for (auto i = 0; i < lookups_threshold_; ++i) {
+ ASSERT_EQ(201, Lookup(200));
+ }
+
+ // Verify entry is upgraded.
+ ASSERT_FALSE(ProbationaryContains(EncodeInt(200)));
+ ASSERT_TRUE(ProtectedContains(EncodeInt(200)));
+ ASSERT_EQ(3, metrics->probationary_segment_evictions.get()->value());
+
+ // Upsert entry in protected segment.
+ Insert(200, 202);
+ auto h4 = slru_cache_->Lookup(EncodeInt(200), Cache::EXPECT_IN_CACHE);
+ ASSERT_EQ(202, DecodeInt(slru_cache_->Value(h4)));
+ ASSERT_EQ(2, evicted_keys_.size());
+ ASSERT_EQ(0, metrics->protected_segment_evictions.get()->value());
+
+ // Reset lookup handle of entry that was upserted, it should be freed now.
+ h3.reset();
+ ASSERT_EQ(3, evicted_keys_.size());
+ ASSERT_EQ(200, evicted_keys_[2]);
+ ASSERT_EQ(201, evicted_values_[2]);
+ ASSERT_EQ(1, metrics->protected_segment_evictions.get()->value());
+
+ // Erase value, lookup handle is still held so entry will not be freed yet.
+ Erase(200);
+ ASSERT_EQ(-1, Lookup(200));
+ ASSERT_EQ(3, evicted_keys_.size());
+ ASSERT_EQ(1, metrics->protected_segment_evictions.get()->value());
+
+ // Reset lookup handle, entry is now freed.
+ h4.reset();
+ ASSERT_EQ(4, evicted_keys_.size());
+ ASSERT_EQ(200, evicted_keys_[3]);
+ ASSERT_EQ(202, evicted_values_[3]);
+ ASSERT_EQ(2, metrics->protected_segment_evictions.get()->value());
}
// Tests that a frequently accessed entry is not evicted.
diff --git a/src/kudu/util/slru_cache.cc b/src/kudu/util/slru_cache.cc
index 333bb9123..bdd5dff06 100644
--- a/src/kudu/util/slru_cache.cc
+++ b/src/kudu/util/slru_cache.cc
@@ -319,6 +319,7 @@ Handle*
SLRUCacheShard<Segment::kProtected>::ProtectedInsert(SLRUHandle* handle,
vector<SLRUHandle*>* evictions) {
handle->eviction_callback = eviction_callback;
handle->refs.store(2, std::memory_order_relaxed);
+ handle->in_protected_segment.store(true, std::memory_order_relaxed);
UpdateMemTracker(handle->charge);
if (PREDICT_TRUE(metrics_)) {
metrics_->cache_usage->IncrementBy(handle->charge);
diff --git a/src/kudu/util/slru_cache.h b/src/kudu/util/slru_cache.h
index dab20c7a5..e1f8e97cb 100644
--- a/src/kudu/util/slru_cache.h
+++ b/src/kudu/util/slru_cache.h
@@ -24,6 +24,8 @@
#include <string>
#include <vector>
+#include <gtest/gtest_prod.h>
+
#include "kudu/gutil/macros.h"
#include "kudu/util/alignment.h"
#include "kudu/util/cache.h"
@@ -267,6 +269,7 @@ class ShardedSLRUCache : public Cache {
private:
friend class SLRUCacheBaseTest;
+ FRIEND_TEST(SLRUCacheTest, EntriesArePinned);
static int DetermineShardBits();
static uint32_t HashSlice(const Slice& s);
