diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index dbac87eb9870b..b82e72df068f5 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -307,18 +307,23 @@ public V computeIfAbsent(ICacheKey key, LoadAwareCacheLoader, V> // Add the value to the onHeap cache. We are calling computeIfAbsent which does another get inside. // This is needed as there can be many requests for the same key at the same time and we only want to load // the value once. - Tuple computedValueTuple = compute(key, loader, future); - // Handle stats - if (computedValueTuple.v2()) { - // The value was just computed and added to the cache by this thread, or it was rejected by the policy. - // Register a miss for the heap cache, and the disk cache if present - statsHolder.incrementMisses(heapDimensionValues); - if (caches.get(diskCache).isEnabled()) { - statsHolder.incrementMisses(diskDimensionValues); + Tuple> computedValueTuple = compute(key, loader, future); + boolean wasCacheMiss = computedValueTuple.v2().v1(); + boolean wasRejectedByPolicy = computedValueTuple.v2().v2(); + // If the value was rejected by policy, it counts as neither a hit or miss. + if (!wasRejectedByPolicy) { + // Handle stats + if (wasCacheMiss) { + // The value was just computed and added to the cache by this thread. + // Register a miss for the heap cache, and the disk cache if present + statsHolder.incrementMisses(heapDimensionValues); + if (caches.get(diskCache).isEnabled()) { + statsHolder.incrementMisses(diskDimensionValues); + } + } else { + // Another thread requesting this key already loaded the value. Register a hit for the heap cache + statsHolder.incrementHits(heapDimensionValues); } - } else { - // Another thread requesting this key already loaded the value. Register a hit for the heap cache - statsHolder.incrementHits(heapDimensionValues); } return computedValueTuple.v1(); } else { @@ -335,7 +340,7 @@ public V computeIfAbsent(ICacheKey key, LoadAwareCacheLoader, V> return cacheValueTuple.v1(); } - private Tuple compute( + private Tuple> compute( ICacheKey key, LoadAwareCacheLoader, V> loader, CompletableFuture, V>> future @@ -343,6 +348,7 @@ private Tuple compute( // Handler to handle results post-processing. Takes a tuple or exception as an input and returns // the value. Also before returning value, puts the value in cache. boolean wasCacheMiss = false; + boolean wasRejectedByPolicy = false; BiFunction, V>, Throwable, Void> handler = (pair, ex) -> { if (pair != null) { boolean didAddToCache = false; @@ -381,14 +387,15 @@ private Tuple compute( future.completeExceptionally(npe); throw new ExecutionException(npe); } else { - wasCacheMiss = true; if (evaluatePoliciesList(value, policies)) { future.complete(new Tuple<>(key, value)); + wasCacheMiss = true; } else { future.complete(null); // Passing null would skip the logic to put this into onHeap cache. // Signal to the caller that the key didn't enter the cache by sending a removal notification. - // This case also counts as a cache miss. + // This case does not count as a cache miss. removalListener.onRemoval(new RemovalNotification<>(key, value, RemovalReason.EXPLICIT)); + wasRejectedByPolicy = true; } } } else { @@ -396,10 +403,10 @@ private Tuple compute( Tuple, V> futureTuple = future.get(); if (futureTuple == null) { // This case can happen if we earlier completed the future with null to skip putting the value into the cache. - // It should behave the same as a cache miss. - wasCacheMiss = true; + // It does not count as a cache miss. value = loader.load(key); removalListener.onRemoval(new RemovalNotification<>(key, value, RemovalReason.EXPLICIT)); + wasRejectedByPolicy = true; } else { value = futureTuple.v2(); } @@ -407,7 +414,7 @@ private Tuple compute( throw new IllegalStateException(ex); } } - return new Tuple<>(value, wasCacheMiss); + return new Tuple<>(value, new Tuple<>(wasCacheMiss, wasRejectedByPolicy)); } @Override diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 0bba92d2b4797..c52551f764437 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -1727,11 +1727,8 @@ public void testEntryPoliciesConcurrentlyWithComputeIfAbsent() throws Exception assertEquals(expectedKeys, getItemsForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_ON_HEAP)); // We should have (numRepetitionsPerKey - 1) * (expectedKeys) hits assertEquals((numRepetitionsPerKey - 1) * expectedKeys, getHitsForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_ON_HEAP)); - // We should have numRepetitionsPerKey misses for each rejected key, and 1 miss for each accepted key - assertEquals( - numRepetitionsPerKey * (keyValuePairs.size() - expectedKeys) + expectedKeys, - getMissesForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_ON_HEAP) - ); + // We should have 1 miss for each accepted key. Rejected keys should not cause misses. + assertEquals(expectedKeys, getMissesForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_ON_HEAP)); for (String key : keyValuePairs.keySet()) { ICacheKey iCacheKey = getICacheKey(key);