chore: Remove share-hits attribute for secondary storage

[1] added a share-hits attribute for secondary storages so that it's
possible to avoid sharing hits to primary storage for a specific
secondary storage. I believe that nobody needs that level of control --
what one would like is the ability to not use the primary storage at
all. Such a feature will be added in a a future commit, but for now the
share-hits=false functionality is just in the way, so let's remove it.

[1]: 1924ad69cc42336a61ee69d0042de53db2a2a52c

(cherry picked from commit fd91ae11db7b49ad26a347f08517b81c7d549153)

diff --git a/doc/MANUAL.adoc b/doc/MANUAL.adoc
index eaa25d985bfa3c4438abf3fdc77abd49914423d2..290d3cf30e311da0e405af99b4c2ed5b4180c130 100644 (file)
--- a/doc/MANUAL.adoc
+++ b/doc/MANUAL.adoc
@@ -1072,8 +1072,6 @@ Examples:
 * `+http://example.com/*|shards=alpha,beta+` will put 50% of the cache on
   `+http://example.com/alpha+` and 50% on `+http://example.com/beta+`.
 --
-* *share-hits*: If *true*, write hits for this backend to primary storage. The
-  default is *true*.
 
 
 === Storage interaction
@@ -1086,14 +1084,13 @@ on cache hits and misses:
 | *Primary storage* | *Secondary storage* | *What happens*
 
 | miss | miss | Compile, write to primary, write to secondary^[1]^
-| miss | hit  | Read from secondary, write to primary^[2]^
-| hit  | -    | Read from primary, don't write to secondary^[3]^
+| miss | hit  | Read from secondary, write to primary
+| hit  | -    | Read from primary, don't write to secondary^[2]^
 
 |==============================================================================
 
 ^[1]^ Unless secondary storage has attribute `read-only=true`. +
-^[2]^ Unless secondary storage has attribute `share-hits=false`. +
-^[3]^ Unless primary storage is set to share its cache hits with the
+^[2]^ Unless primary storage is set to share its cache hits with the
 <<config_reshare,*reshare*>> option.
 
 

diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp
index e3f9b96365ede94eb6e671b0a91dbdb1d4db4cb1..af8c11518cbf178d82c8a80ba5156ac989eb694a 100644 (file)
--- a/src/storage/Storage.cpp
+++ b/src/storage/Storage.cpp
@@ -41,7 +41,6 @@
 
 #include <third_party/url.hpp>
 
-#include <algorithm>
 #include <cmath>
 #include <unordered_map>
 #include <vector>
@@ -83,7 +82,6 @@ struct SecondaryStorageConfig
   std::vector<SecondaryStorageShardConfig> shards;
   secondary::SecondaryStorage::Backend::Params params;
   bool read_only = false;
-  bool share_hits = true;
 };
 
 struct SecondaryStorageBackendEntry
@@ -175,8 +173,6 @@ parse_storage_config(const std::string_view entry)
 
         result.shards.push_back({std::string(name), weight});
       }
-    } else if (key == "share-hits") {
-      result.share_hits = (value == "true");
     }
 
     result.params.attributes.push_back(
@@ -267,33 +263,29 @@ Storage::get(const Digest& key,
     return std::nullopt;
   }
 
-  const auto value_and_share_hits = get_from_secondary_storage(key);
-  if (!value_and_share_hits) {
+  const auto value = get_from_secondary_storage(key);
+  if (!value) {
     return std::nullopt;
   }
-  const auto& value = value_and_share_hits->first;
-  const auto& share_hits = value_and_share_hits->second;
 
   TemporaryFile tmp_file(FMT("{}/tmp.get", m_config.temporary_dir()));
   m_tmp_files.push_back(tmp_file.path);
   try {
-    util::write_file(tmp_file.path, value);
+    util::write_file(tmp_file.path, *value);
   } catch (const core::Error& e) {
     throw core::Fatal(FMT("Error writing to {}: {}", tmp_file.path, e.what()));
   }
 
-  if (share_hits) {
-    primary.put(key, type, [&](const auto& path) {
-      try {
-        Util::ensure_dir_exists(Util::dir_name(path));
-        Util::copy_file(tmp_file.path, path);
-      } catch (const core::Error& e) {
-        LOG("Failed to copy {} to {}: {}", tmp_file.path, path, e.what());
-        // Don't indicate failure since get from primary storage was OK.
-      }
-      return true;
-    });
-  }
+  primary.put(key, type, [&](const auto& path) {
+    try {
+      Util::ensure_dir_exists(Util::dir_name(path));
+      Util::copy_file(tmp_file.path, path);
+    } catch (const core::Error& e) {
+      LOG("Failed to copy {} to {}: {}", tmp_file.path, path, e.what());
+      // Don't indicate failure since get from primary storage was OK.
+    }
+    return true;
+  });
 
   return tmp_file.path;
 }
@@ -477,7 +469,7 @@ Storage::get_backend(SecondaryStorageEntry& entry,
   }
 }
 
-std::optional<std::pair<util::Blob, bool>>
+std::optional<util::Blob>
 Storage::get_from_secondary_storage(const Digest& key)
 {
   MTR_SCOPE("secondary_storage", "get");
@@ -503,7 +495,7 @@ Storage::get_from_secondary_storage(const Digest& key)
           backend->url_for_logging,
           ms);
       primary.increment_statistic(core::Statistic::secondary_storage_hit);
-      return std::make_pair(*value, entry->config.share_hits);
+      return *value;
     } else {
       LOG("No {} in {} ({:.2f} ms)",
           key.to_string(),

diff --git a/src/storage/Storage.hpp b/src/storage/Storage.hpp
index 5d9abd0bf77203faf95aca462fcad624e622acf4..6810fe1300735c598c1340eaba5158e5c3b8e3b7 100644 (file)
--- a/src/storage/Storage.hpp
+++ b/src/storage/Storage.hpp
@@ -28,7 +28,6 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <utility>
 #include <vector>
 
 class Digest;
@@ -82,8 +81,7 @@ private:
               const Digest& key,
               std::string_view operation_description,
               const bool for_writing);
-  std::optional<std::pair<util::Blob, bool>>
-  get_from_secondary_storage(const Digest& key);
+  std::optional<util::Blob> get_from_secondary_storage(const Digest& key);
 
   void put_in_secondary_storage(const Digest& key,
                                 const util::Blob& value,

diff --git a/src/storage/secondary/SecondaryStorage.cpp b/src/storage/secondary/SecondaryStorage.cpp
index e800422179f5426ac9163baa0e8cb2507571feb0..a9ae934fe8c93beca3b709188a19608719d7a33b 100644 (file)
--- a/src/storage/secondary/SecondaryStorage.cpp
+++ b/src/storage/secondary/SecondaryStorage.cpp
@@ -26,7 +26,7 @@ namespace storage::secondary {
 bool
 SecondaryStorage::Backend::is_framework_attribute(const std::string& name)
 {
-  return name == "read-only" || name == "shards" || name == "share-hits";
+  return name == "read-only" || name == "shards";
 }
 
 std::chrono::milliseconds

diff --git a/test/suites/secondary_file.bash b/test/suites/secondary_file.bash
index 1a6886259f51c6d7331f9de63a272a6339f0c3a6..e432dd7e3bf27abe1047e12a83b5401eaa22d811 100644 (file)
--- a/test/suites/secondary_file.bash
+++ b/test/suites/secondary_file.bash
@@ -239,33 +239,6 @@ SUITE_secondary_file() {
     expect_stat secondary_storage_miss 0
     expect_file_count 3 '*' secondary # CACHEDIR.TAG + result + manifest
 
-    # -------------------------------------------------------------------------
-    TEST "Don't share hits"
-
-    $CCACHE_COMPILE -c test.c
-    expect_stat direct_cache_hit 0
-    expect_stat cache_miss 1
-    expect_stat files_in_cache 2
-    expect_stat primary_storage_hit 0
-    expect_stat primary_storage_miss 2
-    expect_stat secondary_storage_hit 0
-    expect_stat secondary_storage_miss 2
-    expect_file_count 3 '*' secondary # CACHEDIR.TAG + result + manifest
-
-    $CCACHE -C >/dev/null
-    expect_stat files_in_cache 0
-
-    CCACHE_SECONDARY_STORAGE+="|share-hits=false"
-    $CCACHE_COMPILE -c test.c
-    expect_stat direct_cache_hit 1
-    expect_stat cache_miss 1
-    expect_stat files_in_cache 0
-    expect_stat primary_storage_hit 0
-    expect_stat primary_storage_miss 4
-    expect_stat secondary_storage_hit 2
-    expect_stat secondary_storage_miss 2
-    expect_file_count 3 '*' secondary # CACHEDIR.TAG + result + manifest
-
     # -------------------------------------------------------------------------
     TEST "Recache"