Skip to content

Commit 39ae32a

Browse files
peteralfonsiPeter Alfonsi
peteralfonsi
and
Peter Alfonsi
authored
[Tiered Caching] Additional ITs for cache stats (opensearch-project#13655)
* Adds cache clear IT Signed-off-by: Peter Alfonsi <petealft@amazon.com> Cleaned up logic for cache stats ITs Signed-off-by: Peter Alfonsi <petealft@amazon.com> Adds more tests around tiered spillover cache Signed-off-by: Peter Alfonsi <petealft@amazon.com> Fixed cache stats behavior for overall /_nodes/stats call Signed-off-by: Peter Alfonsi <petealft@amazon.com> cleanup Signed-off-by: Peter Alfonsi <petealft@amazon.com> Fixed folder structure Signed-off-by: Peter Alfonsi <petealft@amazon.com> Addressed Sagar's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> Addressed Ankit's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> Break horrifyingly long test case into many shorter cases Signed-off-by: Peter Alfonsi <petealft@amazon.com> Added unsupported operation exception to TSC stats holder incrementEvictions() Signed-off-by: Peter Alfonsi <petealft@amazon.com> Addressed Sorabh's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun assemble Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun gradle Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun gradle Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun gradle Signed-off-by: Peter Alfonsi <petealft@amazon.com> --------- Signed-off-by: Peter Alfonsi <petealft@amazon.com> Co-authored-by: Peter Alfonsi <petealft@amazon.com>
1 parent 97340ed commit 39ae32a

File tree

9 files changed

+700
-99
lines changed

9 files changed

+700
-99
lines changed

‎modules/cache-common/src/internalClusterTest/java/org.opensearch.cache.common.tier/TieredSpilloverCacheIT.java ‎modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
6565
return Arrays.asList(TieredSpilloverCachePlugin.class, MockDiskCachePlugin.class);
6666
}
6767

68-
private Settings defaultSettings(String onHeapCacheSizeInBytesOrPecentage) {
68+
static Settings defaultSettings(String onHeapCacheSizeInBytesOrPercentage) {
6969
return Settings.builder()
7070
.put(FeatureFlags.PLUGGABLE_CACHE, "true")
7171
.put(
@@ -88,7 +88,7 @@ private Settings defaultSettings(String onHeapCacheSizeInBytesOrPecentage) {
8888
OpenSearchOnHeapCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE)
8989
.get(MAXIMUM_SIZE_IN_BYTES_KEY)
9090
.getKey(),
91-
onHeapCacheSizeInBytesOrPecentage
91+
onHeapCacheSizeInBytesOrPercentage
9292
)
9393
.build();
9494
}

‎modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java

+501
Large diffs are not rendered by default.

‎modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java

+12-4
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ private Function<ICacheKey<K>, Tuple<V, String>> getValueFromTieredCache(boolean
327327
void handleRemovalFromHeapTier(RemovalNotification<ICacheKey<K>, V> notification) {
328328
ICacheKey<K> key = notification.getKey();
329329
boolean wasEvicted = SPILLOVER_REMOVAL_REASONS.contains(notification.getRemovalReason());
330+
boolean countEvictionTowardsTotal = false; // Don't count this eviction towards the cache's total if it ends up in the disk tier
330331
if (caches.get(diskCache).isEnabled() && wasEvicted && evaluatePolicies(notification.getValue())) {
331332
try (ReleasableLock ignore = writeLock.acquire()) {
332333
diskCache.put(key, notification.getValue()); // spill over to the disk tier and increment its stats
@@ -336,21 +337,28 @@ void handleRemovalFromHeapTier(RemovalNotification<ICacheKey<K>, V> notification
336337
// If the value is not going to the disk cache, send this notification to the TSC's removal listener
337338
// as the value is leaving the TSC entirely
338339
removalListener.onRemoval(notification);
340+
countEvictionTowardsTotal = true;
339341
}
340-
updateStatsOnRemoval(TIER_DIMENSION_VALUE_ON_HEAP, wasEvicted, key, notification.getValue());
342+
updateStatsOnRemoval(TIER_DIMENSION_VALUE_ON_HEAP, wasEvicted, key, notification.getValue(), countEvictionTowardsTotal);
341343
}
342344

343345
void handleRemovalFromDiskTier(RemovalNotification<ICacheKey<K>, V> notification) {
344346
// Values removed from the disk tier leave the TSC entirely
345347
removalListener.onRemoval(notification);
346348
boolean wasEvicted = SPILLOVER_REMOVAL_REASONS.contains(notification.getRemovalReason());
347-
updateStatsOnRemoval(TIER_DIMENSION_VALUE_DISK, wasEvicted, notification.getKey(), notification.getValue());
349+
updateStatsOnRemoval(TIER_DIMENSION_VALUE_DISK, wasEvicted, notification.getKey(), notification.getValue(), true);
348350
}
349351

350-
void updateStatsOnRemoval(String removedFromTierValue, boolean wasEvicted, ICacheKey<K> key, V value) {
352+
void updateStatsOnRemoval(
353+
String removedFromTierValue,
354+
boolean wasEvicted,
355+
ICacheKey<K> key,
356+
V value,
357+
boolean countEvictionTowardsTotal
358+
) {
351359
List<String> dimensionValues = statsHolder.getDimensionsWithTierValue(key.dimensions, removedFromTierValue);
352360
if (wasEvicted) {
353-
statsHolder.incrementEvictions(dimensionValues);
361+
statsHolder.incrementEvictions(dimensionValues, countEvictionTowardsTotal);
354362
}
355363
statsHolder.decrementItems(dimensionValues);
356364
statsHolder.decrementSizeInBytes(dimensionValues, weigher.applyAsLong(key, value));

‎modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java

+19-10
Original file line numberDiff line numberDiff line change
@@ -105,20 +105,29 @@ public void incrementMisses(List<String> dimensionValues) {
105105
internalIncrement(dimensionValues, missIncrementer, true);
106106
}
107107

108+
/**
109+
* This method shouldn't be used in this class. Instead, use incrementEvictions(dimensionValues, includeInTotal)
110+
* which specifies whether the eviction should be included in the cache's total evictions, or if it should
111+
* just count towards that tier's evictions.
112+
* @param dimensionValues The dimension values
113+
*/
108114
@Override
109115
public void incrementEvictions(List<String> dimensionValues) {
110-
final String tierValue = validateTierDimensionValue(dimensionValues);
116+
throw new UnsupportedOperationException(
117+
"TieredSpilloverCacheHolder must specify whether to include an eviction in the total cache stats. Use incrementEvictions(List<String> dimensionValues, boolean includeInTotal)"
118+
);
119+
}
111120

112-
// If the disk tier is present, only evictions from the disk tier should be included in total values.
121+
/**
122+
* Increment evictions for this set of dimension values.
123+
* @param dimensionValues The dimension values
124+
* @param includeInTotal Whether to include this eviction in the total for the whole cache's evictions
125+
*/
126+
public void incrementEvictions(List<String> dimensionValues, boolean includeInTotal) {
127+
validateTierDimensionValue(dimensionValues);
128+
// If we count this eviction towards the total, we should increment all ancestor nodes. If not, only increment the leaf node.
113129
Consumer<DefaultCacheStatsHolder.Node> evictionsIncrementer = (node) -> {
114-
if (tierValue.equals(TIER_DIMENSION_VALUE_ON_HEAP) && diskCacheEnabled) {
115-
// If on-heap tier, increment only the leaf node corresponding to the on heap values; not the total values in its parent
116-
// nodes
117-
if (node.isAtLowestLevel()) {
118-
node.incrementEvictions();
119-
}
120-
} else {
121-
// If disk tier, or on-heap tier with a disabled disk tier, increment the leaf node and its parents
130+
if (includeInTotal || node.isAtLowestLevel()) {
122131
node.incrementEvictions();
123132
}
124133
};

‎modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java

+15-2
Original file line numberDiff line numberDiff line change
@@ -926,14 +926,14 @@ public void testDiskTierPolicies() throws Exception {
926926
MockCacheRemovalListener<String, String> removalListener = new MockCacheRemovalListener<>();
927927
TieredSpilloverCache<String, String> tieredSpilloverCache = intializeTieredSpilloverCache(
928928
keyValueSize,
929-
100,
929+
keyValueSize * 100,
930930
removalListener,
931931
Settings.builder()
932932
.put(
933933
OpenSearchOnHeapCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE)
934934
.get(MAXIMUM_SIZE_IN_BYTES_KEY)
935935
.getKey(),
936-
onHeapCacheSize * 50 + "b"
936+
onHeapCacheSize * keyValueSize + "b"
937937
)
938938
.build(),
939939
0,
@@ -955,6 +955,7 @@ public void testDiskTierPolicies() throws Exception {
955955

956956
LoadAwareCacheLoader<ICacheKey<String>, String> loader = getLoadAwareCacheLoader(keyValuePairs);
957957

958+
int expectedEvictions = 0;
958959
for (String key : keyValuePairs.keySet()) {
959960
ICacheKey<String> iCacheKey = getICacheKey(key);
960961
Boolean expectedOutput = expectedOutputs.get(key);
@@ -967,8 +968,15 @@ public void testDiskTierPolicies() throws Exception {
967968
} else {
968969
// Should miss as heap tier size = 0 and the policy rejected it
969970
assertNull(result);
971+
expectedEvictions++;
970972
}
971973
}
974+
975+
// We expect values that were evicted from the heap tier and not allowed into the disk tier by the policy
976+
// to count towards total evictions
977+
assertEquals(keyValuePairs.size(), getEvictionsForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_ON_HEAP));
978+
assertEquals(0, getEvictionsForTier(tieredSpilloverCache, TIER_DIMENSION_VALUE_DISK)); // Disk tier is large enough for no evictions
979+
assertEquals(expectedEvictions, getTotalStatsSnapshot(tieredSpilloverCache).getEvictions());
972980
}
973981

974982
public void testTookTimePolicyFromFactory() throws Exception {
@@ -1493,6 +1501,11 @@ private ImmutableCacheStats getStatsSnapshotForTier(TieredSpilloverCache<?, ?> t
14931501
return snapshot;
14941502
}
14951503

1504+
private ImmutableCacheStats getTotalStatsSnapshot(TieredSpilloverCache<?, ?> tsc) throws IOException {
1505+
ImmutableCacheStatsHolder cacheStats = tsc.stats(new String[0]);
1506+
return cacheStats.getStatsForDimensionValues(List.of());
1507+
}
1508+
14961509
// Duplicated here from EhcacheDiskCacheTests.java, we can't add a dependency on that plugin
14971510
static class StringSerializer implements Serializer<String, byte[]> {
14981511
private final Charset charset = StandardCharsets.UTF_8;

0 commit comments

Comments
 (0)