fix: do not upload offload values on a first hit

src/server/db_slice.cc

@@ -506,7 +506,7 @@ OpResult<DbSlice::PrimeItAndExp> DbSlice::FindInternal(const Context& cntx, std:
   }
 
   // Mark this entry as being looked up. We use key (first) deliberately to preserve the hotness
-  // attribute of the entry in case of value overtides.
+  // attribute of the entry in case of value overrides.
   res.it->first.SetTouched(true);
 
   db.top_keys.Touch(key);

src/server/tiered_storage.cc

@@ -214,10 +214,14 @@ bool TieredStorage::ShardOpManager::NotifyFetched(EntryId id, string_view value,
   auto key = get<OpManager::KeyRef>(id);
   auto* pv = Find(key);
   if (pv && pv->IsExternal() && segment == pv->GetExternalSlice()) {
-    bool is_raw = !modified;
-    ++stats_.total_uploads;
-    Upload(key.first, value, is_raw, segment.length, pv);
-    return true;
+    if (modified || pv->WasTouched()) {
+      bool is_raw = !modified;
+      ++stats_.total_uploads;
+      Upload(key.first, value, is_raw, segment.length, pv);
+      return true;
+    }
+    pv->SetTouched(true);
+    return false;
   }
 
   LOG(DFATAL) << "Internal error, should not reach this";

src/server/tiered_storage_test.cc

@@ -90,6 +90,7 @@ TEST_F(TieredStorageTest, SimpleGetSet) {
   for (size_t i = kMin; i < kMax; i++) {
     auto resp = Run({"GET", absl::StrCat("k", i)});
     ASSERT_EQ(resp, string(i, 'B')) << i;
+    Run({"GET", absl::StrCat("k", i)});  // To enforce uploads.
   }
 
   metrics = GetMetrics();
@@ -137,7 +138,9 @@ TEST_F(TieredStorageTest, MultiDb) {
   for (size_t i = 0; i < 10; i++) {
     Run({"SELECT", absl::StrCat(i)});
     EXPECT_EQ(GetMetrics().db_stats[i].tiered_entries, 1);
-    EXPECT_EQ(Run({"GET", absl::StrCat("k", i)}), BuildString(3000, char('A' + i)));
+    string key = absl::StrCat("k", i);
+    EXPECT_EQ(Run({"GET", key}), BuildString(3000, char('A' + i)));
+    Run({"GET", key});
     EXPECT_EQ(GetMetrics().db_stats[i].tiered_entries, 0);
   }
 }
@@ -158,15 +161,18 @@ TEST_F(TieredStorageTest, Defrag) {
   ASSERT_EQ(metrics.tiered_stats.small_bins_filling_bytes, 537);
 
   // Reading 3 values still leaves the bin more than half occupied
-  Run({"GET", string(1, 'a')});
-  Run({"GET", string(1, 'b')});
-  Run({"GET", string(1, 'c')});
+  for (unsigned j = 0; j < 2; ++j) {
+    Run({"GET", string(1, 'a')});
+    Run({"GET", string(1, 'b')});
+    Run({"GET", string(1, 'c')});
+  }
   metrics = GetMetrics();
   EXPECT_EQ(metrics.tiered_stats.small_bins_cnt, 1u);
   EXPECT_EQ(metrics.tiered_stats.small_bins_entries_cnt, 4u);
 
   // This tirggers defragmentation, as only 3 < 7/2 remain left
   Run({"GET", string(1, 'd')});
+  Run({"GET", string(1, 'd')});
   metrics = GetMetrics();
   EXPECT_EQ(metrics.tiered_stats.total_defrags, 3u);
   EXPECT_EQ(metrics.tiered_stats.small_bins_cnt, 0u);
@@ -202,6 +208,7 @@ TEST_F(TieredStorageTest, BackgroundOffloading) {
     EXPECT_EQ(resp, value);
     resp = Run({"TTL", key});
     EXPECT_THAT(resp, IntArg(100));
+    Run({"GET", key});  // enforce uploads
   }
 
   // Wait for offload to do it all again
@@ -214,7 +221,7 @@ TEST_F(TieredStorageTest, BackgroundOffloading) {
   // should be re-stashed again.
   EXPECT_EQ(metrics.tiered_stats.total_stashes, kNum + metrics.tiered_stats.total_uploads)
       << resp.GetString();
-  EXPECT_EQ(metrics.tiered_stats.total_fetches, kNum);
+  EXPECT_EQ(metrics.tiered_stats.total_fetches, kNum * 2);
   EXPECT_EQ(metrics.tiered_stats.allocated_bytes, kNum * 4096);
 }
 

src/server/tiering/op_manager.h

@@ -76,7 +76,7 @@ class OpManager {
   virtual void NotifyStashed(EntryId id, const io::Result<DiskSegment>& segment) = 0;
 
   // Notify that an entry was successfully fetched. Includes whether entry was modified.
-  // Returns true if value needs to be deleted.
+  // Returns true if value needs to be deleted from the storage.
   virtual bool NotifyFetched(EntryId id, std::string_view value, DiskSegment segment,
                              bool modified) = 0;