From ee1eda99212636285f92745140c625c152f6b04a Mon Sep 17 00:00:00 2001
From: Christian Schwarz <christian@neon.tech>
Date: Mon, 20 Feb 2023 16:29:39 +0100
Subject: [PATCH] eviction: remove
 EvictionStats::not_considered_due_to_clock_skew

Rationale: see the block comment added in this patch.

fixes #3641
---
 .../src/tenant/timeline/eviction_task.rs      | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs
index e3e7ce4c9d..fc84517cc2 100644
--- a/pageserver/src/tenant/timeline/eviction_task.rs
+++ b/pageserver/src/tenant/timeline/eviction_task.rs
@@ -100,7 +100,6 @@ impl Timeline {
         #[allow(dead_code)]
         #[derive(Debug, Default)]
         struct EvictionStats {
-            not_considered_due_to_clock_skew: usize,
             candidates: usize,
             evicted: usize,
             errors: usize,
@@ -129,9 +128,21 @@ impl Timeline {
                 let no_activity_for = match now.duration_since(last_activity_ts) {
                     Ok(d) => d,
                     Err(_e) => {
-                        // NB: don't log the error. If there are many layers and the system clock
-                        // is skewed, we'd be flooding the log.
-                        stats.not_considered_due_to_clock_skew += 1;
+                        // We reach here if `now` < `last_activity_ts`, which can legitimately
+                        // happen if there is an access between us getting `now`, and us getting
+                        // the access stats from the layer.
+                        //
+                        // The other reason why it can happen is system clock skew because
+                        // SystemTime::now() is not monotonic, so, even if there is no access
+                        // to the layer after we get `now` at the beginning of this function,
+                        // it could be that `now`  < `last_activity_ts`.
+                        //
+                        // To distinguish the cases, we would need to record `Instant`s in the
+                        // access stats (i.e., monotonic timestamps), but then, the timestamps
+                        // values in the access stats would need to be `Instant`'s, and hence
+                        // they would be meaningless outside of the pageserver process.
+                        // At the time of writing, the trade-off is that access stats are more
+                        // valuable than detecting clock skew.
                         continue;
                     }
                 };
@@ -188,8 +199,7 @@ impl Timeline {
                 }
             }
         }
-        if stats.not_considered_due_to_clock_skew > 0 || stats.errors > 0 || stats.not_evictable > 0
-        {
+        if stats.errors > 0 || stats.not_evictable > 0 {
             warn!(stats=?stats, "eviction iteration complete");
         } else {
             info!(stats=?stats, "eviction iteration complete");