From fbe23f1b598b125ecced5bc6a19c3cfafe63ef99 Mon Sep 17 00:00:00 2001
From: Mark Juggurnauth-Thomas <mbthomas@meta.com>
Date: Thu, 23 Feb 2023 11:51:31 -0800
Subject: [PATCH] caching_ext: move spawn_memcache_writes determination into
 memcache handler

Summary: The `spawn_memcache_writes` feature used by changesets and the new commit graph should be part of the memcache handler, rather than externally implemented.  Essentially we want the real memcache handler to always spawn writes, however when dealing with mocks or no-ops, there is no need to spawn, and in fact spawning would be harmful by making the test less deterministic.

Differential Revision: D43410854

fbshipit-source-id: 0be62a4b49fcc0bd62aabba1d65d10f6d16e7129
---
 .../changesets/changesets_impl/src/caching.rs | 10 ---------
 .../common/rust/caching_ext/src/lib.rs        | 21 ++-----------------
 .../rust/caching_ext/src/memcache_utils.rs    |  7 +++++++
 .../caching_commit_graph_storage/src/lib.rs   |  8 -------
 4 files changed, 9 insertions(+), 37 deletions(-)

diff --git a/eden/mononoke/changesets/changesets_impl/src/caching.rs b/eden/mononoke/changesets/changesets_impl/src/caching.rs
index 4f1de574cc..8c24fde0aa 100644
--- a/eden/mononoke/changesets/changesets_impl/src/caching.rs
+++ b/eden/mononoke/changesets/changesets_impl/src/caching.rs
@@ -279,16 +279,6 @@ impl EntityStore<ChangesetEntryWrapper> for CacheRequest<'_> {
     }
 
     caching_ext::impl_singleton_stats!("changesets");
-
-    #[cfg(test)]
-    fn spawn_memcache_writes(&self) -> bool {
-        let (_, mapping) = self;
-
-        match mapping.memcache {
-            MemcacheHandler::Real(_) => true,
-            MemcacheHandler::Mock(..) | MemcacheHandler::Noop => false,
-        }
-    }
 }
 
 #[async_trait]
diff --git a/eden/mononoke/common/rust/caching_ext/src/lib.rs b/eden/mononoke/common/rust/caching_ext/src/lib.rs
index 6cba4e641d..05e21c3506 100644
--- a/eden/mononoke/common/rust/caching_ext/src/lib.rs
+++ b/eden/mononoke/common/rust/caching_ext/src/lib.rs
@@ -131,13 +131,6 @@ pub trait EntityStore<V> {
     ///
     /// Implement this method with `caching_ext::impl_singleton_stats!` macro, instead of by hand
     fn stats(&self) -> &CacheStats;
-
-    /// Whether Memcache writes should run in the background. This is normally the desired behavior
-    /// so this defaults to true, but for tests it's useful to run them synchronously to get
-    /// consistent outcomes.
-    fn spawn_memcache_writes(&self) -> bool {
-        true
-    }
 }
 
 /// Implement this to make it possible to fetch keys via the cache
@@ -352,12 +345,7 @@ async fn fill_caches_by_key<'a, V>(
 
     fill_multiple_cachelib(store.cachelib(), cachelib_keys);
 
-    fill_multiple_memcache(
-        store.memcache(),
-        memcache_keys,
-        store.spawn_memcache_writes(),
-    )
-    .await;
+    fill_multiple_memcache(store.memcache(), memcache_keys).await;
 }
 
 async fn get_multiple_from_memcache<K, V>(
@@ -435,7 +423,6 @@ fn fill_multiple_cachelib<'a, V>(
 async fn fill_multiple_memcache<'a, V: 'a>(
     memcache: &'a MemcacheHandler,
     data: impl IntoIterator<Item = (MemcacheKey, CacheTtl, &'a V)>,
-    spawn: bool,
 ) where
     V: MemcacheEntity,
 {
@@ -465,7 +452,7 @@ async fn fill_multiple_memcache<'a, V: 'a>(
 
     let fut = stream::iter(futs).for_each_concurrent(MEMCACHE_CONCURRENCY, |fut| fut);
 
-    if spawn {
+    if memcache.is_async() {
         tokio::task::spawn(fut);
     } else {
         fut.await;
@@ -536,10 +523,6 @@ mod test {
         }
 
         impl_singleton_stats!("test");
-
-        fn spawn_memcache_writes(&self) -> bool {
-            false
-        }
     }
 
     #[async_trait]
diff --git a/eden/mononoke/common/rust/caching_ext/src/memcache_utils.rs b/eden/mononoke/common/rust/caching_ext/src/memcache_utils.rs
index 9e85531674..62d126c0ca 100644
--- a/eden/mononoke/common/rust/caching_ext/src/memcache_utils.rs
+++ b/eden/mononoke/common/rust/caching_ext/src/memcache_utils.rs
@@ -37,6 +37,13 @@ impl MemcacheHandler {
         }
     }
 
+    pub fn is_async(&self) -> bool {
+        match self {
+            MemcacheHandler::Real(_) => true,
+            MemcacheHandler::Mock(_) | MemcacheHandler::Noop => false,
+        }
+    }
+
     pub async fn get(&self, key: String) -> Result<Option<Bytes>> {
         match self {
             MemcacheHandler::Real(ref client) => {
diff --git a/eden/mononoke/repo_attributes/commit_graph/caching_commit_graph_storage/src/lib.rs b/eden/mononoke/repo_attributes/commit_graph/caching_commit_graph_storage/src/lib.rs
index 5186c1d164..a7cb8f7069 100644
--- a/eden/mononoke/repo_attributes/commit_graph/caching_commit_graph_storage/src/lib.rs
+++ b/eden/mononoke/repo_attributes/commit_graph/caching_commit_graph_storage/src/lib.rs
@@ -181,14 +181,6 @@ impl EntityStore<CachedChangesetEdges> for CacheRequest<'_> {
     }
 
     caching_ext::impl_singleton_stats!("commit_graph");
-
-    #[cfg(test)]
-    fn spawn_memcache_writes(&self) -> bool {
-        match self.caching_storage.memcache {
-            MemcacheHandler::Real(_) => true,
-            MemcacheHandler::Mock(..) | MemcacheHandler::Noop => false,
-        }
-    }
 }
 
 #[async_trait]