diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java index e25fb0112ca01..5063272ff5816 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -70,7 +70,6 @@ public void testDiskTierStats() throws Exception { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello" + 0)).get(); int requestSize = (int) getCacheSizeBytes(client, "index", TierType.ON_HEAP); - System.out.println(requestSize); assertTrue(heapSizeBytes > requestSize); // If this fails, increase heapSizeBytes! We can't adjust it after getting the size of one query // as the cache size setting is not dynamic @@ -82,6 +81,7 @@ public void testDiskTierStats() throws Exception { assertSearchResponse(resp); IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.DISK, false); + assertPositiveEWMAForDisk(client, "index"); } // the first request, for "hello0", should have been evicted to the disk tier resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello0")).get(); @@ -99,4 +99,15 @@ private long getCacheSizeBytes(Client client, String index, TierType tierType) { .getRequestCache(); return requestCacheStats.getMemorySizeInBytes(tierType); } + + private void assertPositiveEWMAForDisk(Client client, String index) { + RequestCacheStats requestCacheStats = client.admin() + .indices() + .prepareStats(index) + .setRequestCache(true) + .get() + .getTotal() + .getRequestCache(); + assertTrue(requestCacheStats.getTimeEWMA(TierType.DISK) > 0); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 080b3017c4246..edb6a963f89c0 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -43,7 +43,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.time.DateFormatter; import org.opensearch.common.util.FeatureFlags; -import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.cache.request.RequestCacheStats; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.aggregations.bucket.global.GlobalAggregationBuilder; diff --git a/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java b/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java index 34d91be1bc2c6..a557438170f48 100644 --- a/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java +++ b/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java @@ -52,12 +52,13 @@ */ public class RequestCacheStats implements Writeable, ToXContentFragment { - private Map map = new HashMap<>(){{ - for (TierType tierType : TierType.values()) + private Map map = new HashMap<>() { { - put(tierType.getStringValue(), new StatsHolder()); - // Every possible tier type must have counters, even if they are disabled. Then the counters report 0 - }} + for (TierType tierType : TierType.values()) { + put(tierType.getStringValue(), new StatsHolder()); + // Every possible tier type must have counters, even if they are disabled. Then the counters report 0 + } + } }; public RequestCacheStats() {} @@ -66,12 +67,12 @@ public RequestCacheStats(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_3_0_0)) { this.map = in.readMap(StreamInput::readString, StatsHolder::new); } else { - // objects from earlier versions only contain on-heap info, and do not have entries info + // objects from earlier versions only contain on-heap info, and do not have entries or getTime info long memorySize = in.readVLong(); long evictions = in.readVLong(); long hitCount = in.readVLong(); long missCount = in.readVLong(); - this.map.put(TierType.ON_HEAP.getStringValue(), new StatsHolder(memorySize, evictions, hitCount, missCount, 0)); + this.map.put(TierType.ON_HEAP.getStringValue(), new StatsHolder(memorySize, evictions, hitCount, missCount, 0, 0.0)); } } @@ -116,6 +117,10 @@ public long getEntries(TierType tierType) { return getTierStats(tierType).entries.count(); } + public double getTimeEWMA(TierType tierType) { + return getTierStats(tierType).getTimeEWMA; + } + // By default, return on-heap stats if no tier is specified public long getMemorySizeInBytes() { @@ -142,12 +147,14 @@ public long getEntries() { return getEntries(TierType.ON_HEAP); } + // no getTimeEWMA default as it'll always return 0 for on-heap + @Override public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_3_0_0)) { out.writeMap(this.map, StreamOutput::writeString, (o, v) -> v.writeTo(o)); // ? } else { - // Write only on-heap values, and don't write entries metric + // Write only on-heap values, and don't write entries metric or getTimeEWMA StatsHolder heapStats = map.get(TierType.ON_HEAP.getStringValue()); out.writeVLong(heapStats.getMemorySize()); out.writeVLong(heapStats.getEvictions()); @@ -160,13 +167,13 @@ public void writeTo(StreamOutput out) throws IOException { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(Fields.REQUEST_CACHE_STATS); // write on-heap stats outside of tiers object - getTierStats(TierType.ON_HEAP).toXContent(builder, params); + getTierStats(TierType.ON_HEAP).toXContent(builder, params, false); // Heap tier doesn't write a getTime builder.startObject(Fields.TIERS); for (TierType tierType : TierType.values()) { // fixed order if (tierType != TierType.ON_HEAP) { String tier = tierType.getStringValue(); builder.startObject(tier); - map.get(tier).toXContent(builder, params); + map.get(tier).toXContent(builder, params, true); // Non-heap tiers write a getTime builder.endObject(); } } @@ -189,5 +196,6 @@ static final class Fields { static final String HIT_COUNT = "hit_count"; static final String MISS_COUNT = "miss_count"; static final String ENTRIES = "entries"; + static final String GET_TIME_EWMA = "get_time_ewma_millis"; } } diff --git a/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java b/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java index b72a4d08e1d99..02ba13d19dd64 100644 --- a/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java +++ b/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java @@ -57,12 +57,19 @@ public RequestCacheStats stats() { return new RequestCacheStats(statsHolder); } - public void onHit(TierType tierType) { + public void onHit(TierType tierType, double getTimeEWMA) { statsHolder.get(tierType).hitCount.inc(); + if (tierType == TierType.DISK) { + statsHolder.get(tierType).getTimeEWMA = getTimeEWMA; + } + } - public void onMiss(TierType tierType) { + public void onMiss(TierType tierType, double getTimeEWMA) { statsHolder.get(tierType).missCount.inc(); + if (tierType == TierType.DISK) { + statsHolder.get(tierType).getTimeEWMA = getTimeEWMA; + } } public void onCached(Accountable key, BytesReference value, TierType tierType) { diff --git a/server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java b/server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java index 92d057ab8fd9c..4d856e052fcfb 100644 --- a/server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java +++ b/server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java @@ -25,6 +25,7 @@ public class StatsHolder implements Serializable, Writeable, ToXContentFragment final CounterMetric hitCount; final CounterMetric missCount; final CounterMetric entries; + double getTimeEWMA; // CounterMetric is long, we need a double public StatsHolder() { this.totalMetric = new CounterMetric(); @@ -32,9 +33,10 @@ public StatsHolder() { this.hitCount = new CounterMetric(); this.missCount = new CounterMetric(); this.entries = new CounterMetric(); + this.getTimeEWMA = 0.0; } - public StatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries) { + public StatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries, double getTimeEWMA) { // Switched argument order to match RequestCacheStats this.totalMetric = new CounterMetric(); this.totalMetric.inc(memorySize); @@ -46,12 +48,13 @@ public StatsHolder(long memorySize, long evictions, long hitCount, long missCoun this.missCount.inc(missCount); this.entries = new CounterMetric(); this.entries.inc(entries); + this.getTimeEWMA = getTimeEWMA; } public StatsHolder(StreamInput in) throws IOException { // Read and write the values of the counter metrics. They should always be positive // This object is new, so we shouldn't need version checks for different behavior - this(in.readVLong(), in.readVLong(), in.readVLong(), in.readVLong(), in.readVLong()); + this(in.readVLong(), in.readVLong(), in.readVLong(), in.readVLong(), in.readVLong(), in.readDouble()); // java forces us to do this in one line // guaranteed to be evaluated in correct order (https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.7.4) } @@ -63,6 +66,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeVLong(hitCount.count()); out.writeVLong(missCount.count()); out.writeVLong(entries.count()); + out.writeDouble(getTimeEWMA); } public void add(StatsHolder otherStats) { @@ -72,6 +76,18 @@ public void add(StatsHolder otherStats) { hitCount.inc(otherStats.hitCount.count()); missCount.inc(otherStats.missCount.count()); entries.inc(otherStats.entries.count()); + if (!otherStats.isEmpty()) { + getTimeEWMA = otherStats.getTimeEWMA; + } + + /* Adding two EWMAs is a bit tricky. If both stats are non-empty we can assume the newer one dominates. + add() is only called in CommonStats.java in two places: + 1) it's used to either add otherStats to a new (empty) RequestCacheStats + 2) it's used to add new stats to an existing RequestCacheStats + In both cases, the existing object is older, so we can assume otherStats's EWMA dominates. + It doesn't make sense to use the existing EWMA in case 1, and in case 2 the actual value + will be updated from the disk tier on the next hit/miss, so it's probably ok to use otherStats.getTimeEWMA. + */ } public long getEvictions() { @@ -94,8 +110,16 @@ public long getEntries() { return entries.count(); } + public double getTimeEWMA() { + return getTimeEWMA; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return toXContent(builder, params, false); // By default do not write the getTime field + } + + public XContentBuilder toXContent(XContentBuilder builder, Params params, boolean includeGetTime) throws IOException { builder.humanReadableField( RequestCacheStats.Fields.MEMORY_SIZE_IN_BYTES, RequestCacheStats.Fields.MEMORY_SIZE, @@ -105,6 +129,18 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(RequestCacheStats.Fields.HIT_COUNT, getHitCount()); builder.field(RequestCacheStats.Fields.MISS_COUNT, getMissCount()); builder.field(RequestCacheStats.Fields.ENTRIES, getEntries()); + if (includeGetTime) { + builder.field(RequestCacheStats.Fields.GET_TIME_EWMA, getTimeEWMA()); + } return builder; } + + private boolean isEmpty() { + return (getEvictions() == 0) + && (getMemorySize() == 0) + && (getHitCount() == 0) + && (getMissCount() == 0) + && (getEntries() == 0) + && (getTimeEWMA() == 0.0); + } } diff --git a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java index 2eef16df2bb9a..6c066b25995a6 100644 --- a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java +++ b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java @@ -56,13 +56,13 @@ public final void onCached(IndicesRequestCache.Key key, BytesReference value, Ti } @Override - public final void onHit(TierType tierType) { - stats().onHit(tierType); + public final void onHit(TierType tierType, double getTimeEWMA) { + stats().onHit(tierType, getTimeEWMA); } @Override - public final void onMiss(TierType tierType) { - stats().onMiss(tierType); + public final void onMiss(TierType tierType, double getTimeEWMA) { + stats().onMiss(tierType, getTimeEWMA); } @Override diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 860875024b18f..7f2d8768304e8 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -151,8 +151,8 @@ void clear(CacheEntity entity) { } @Override - public void onMiss(Key key, TierType tierType) { - key.entity.onMiss(tierType); + public void onMiss(Key key, TierType tierType, double getTimeEWMA) { + key.entity.onMiss(tierType, getTimeEWMA); } @Override @@ -161,8 +161,8 @@ public void onRemoval(RemovalNotification notification) { } @Override - public void onHit(Key key, BytesReference value, TierType tierType) { - key.entity.onHit(tierType); + public void onHit(Key key, BytesReference value, TierType tierType, double getTimeEWMA) { + key.entity.onHit(tierType, getTimeEWMA); } @Override @@ -275,12 +275,12 @@ interface CacheEntity extends Accountable, Writeable { /** * Called each time this entity has a cache hit. */ - void onHit(TierType tierType); + void onHit(TierType tierType, double getTimeEWMA); /** * Called each time this entity has a cache miss. */ - void onMiss(TierType tierType); + void onMiss(TierType tierType, double getTimeEWMA); /** * Called when this entity instance is removed diff --git a/server/src/main/java/org/opensearch/indices/TieredCacheEventListener.java b/server/src/main/java/org/opensearch/indices/TieredCacheEventListener.java index 084ac5a57e0d3..3634a6fa53543 100644 --- a/server/src/main/java/org/opensearch/indices/TieredCacheEventListener.java +++ b/server/src/main/java/org/opensearch/indices/TieredCacheEventListener.java @@ -12,11 +12,11 @@ public interface TieredCacheEventListener { - void onMiss(K key, TierType tierType); + void onMiss(K key, TierType tierType, double getTimeEWMA); void onRemoval(RemovalNotification notification); - void onHit(K key, V value, TierType tierType); + void onHit(K key, V value, TierType tierType, double getTimeEWMA); void onCached(K key, V value, TierType tierType); } diff --git a/server/src/main/java/org/opensearch/indices/TieredCacheSpilloverStrategyHandler.java b/server/src/main/java/org/opensearch/indices/TieredCacheSpilloverStrategyHandler.java index f02a193c7d354..e78678994a612 100644 --- a/server/src/main/java/org/opensearch/indices/TieredCacheSpilloverStrategyHandler.java +++ b/server/src/main/java/org/opensearch/indices/TieredCacheSpilloverStrategyHandler.java @@ -131,16 +131,24 @@ private Function> getValueFromTierCache() { return key -> { for (CachingTier cachingTier : cachingTierList) { V value = cachingTier.get(key); + double getTimeEWMA = getTimeEWMAIfDisk(cachingTier); if (value != null) { - tieredCacheEventListener.onHit(key, value, cachingTier.getTierType()); + tieredCacheEventListener.onHit(key, value, cachingTier.getTierType(), getTimeEWMA); return new CacheValue<>(value, cachingTier.getTierType()); } - tieredCacheEventListener.onMiss(key, cachingTier.getTierType()); + tieredCacheEventListener.onMiss(key, cachingTier.getTierType(), getTimeEWMA); } return null; }; } + private double getTimeEWMAIfDisk(CachingTier cachingTier) { + if (cachingTier.getTierType() == TierType.DISK) { + return ((DiskCachingTier) cachingTier).getTimeMillisEWMA(); + } + return 0.0; + } + @Override public void closeDiskTier() { diskCachingTier.close(); diff --git a/server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java b/server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java index 6992b8a441c0a..f1ae3dbc2273f 100644 --- a/server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java +++ b/server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java @@ -21,29 +21,38 @@ public class RequestCacheStatsTests extends OpenSearchTestCase { public void testConstructorsAndAdd() throws Exception { RequestCacheStats emptyStats = new RequestCacheStats(); for (TierType tierType : TierType.values()) { - assertTierState(emptyStats, tierType, 0, 0, 0, 0, 0); + assertTierState(emptyStats, tierType, 0, 0, 0, 0, 0, 0.0); } Map testHeapMap = new HashMap<>(); - testHeapMap.put(TierType.ON_HEAP, new StatsHolder(1, 2, 3, 4, 5)); + testHeapMap.put(TierType.ON_HEAP, new StatsHolder(1, 2, 3, 4, 5, 0.0)); RequestCacheStats heapOnlyStats = new RequestCacheStats(testHeapMap); for (TierType tierType : TierType.values()) { if (tierType == TierType.ON_HEAP) { - assertTierState(heapOnlyStats, tierType, 1, 2, 3, 4, 5); + assertTierState(heapOnlyStats, tierType, 1, 2, 3, 4, 5, 0.0); } else { - assertTierState(heapOnlyStats, tierType, 0, 0, 0, 0, 0); + assertTierState(heapOnlyStats, tierType, 0, 0, 0, 0, 0, 0.0); } } Map testBothTiersMap = new HashMap<>(); - testBothTiersMap.put(TierType.ON_HEAP, new StatsHolder(11, 12, 13, 14, 15)); - testBothTiersMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10)); + testBothTiersMap.put(TierType.ON_HEAP, new StatsHolder(11, 12, 13, 14, 15, 0.0)); + testBothTiersMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10, 25.0)); RequestCacheStats bothTiersStats = new RequestCacheStats(testBothTiersMap); - assertTierState(bothTiersStats, TierType.ON_HEAP, 11, 12, 13, 14, 15); - assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); + assertTierState(bothTiersStats, TierType.ON_HEAP, 11, 12, 13, 14, 15, 0.0); + assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10, 25.0); bothTiersStats.add(heapOnlyStats); - assertTierState(bothTiersStats, TierType.ON_HEAP, 12, 14, 16, 18, 20); - assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); + assertTierState(bothTiersStats, TierType.ON_HEAP, 12, 14, 16, 18, 20, 0.0); + assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10, 25.0); + + Map addEWMAMap = new HashMap<>(); + addEWMAMap.put(TierType.DISK, new StatsHolder(1, 1, 1, 1, 1, 16.0)); + RequestCacheStats addEWMAStats = new RequestCacheStats(addEWMAMap); + bothTiersStats.add(addEWMAStats); + assertTierState(bothTiersStats, TierType.ON_HEAP, 12, 14, 16, 18, 20, 0.0); + assertTierState(bothTiersStats, TierType.DISK, 7, 8, 9, 10, 11, 16.0); + // The new EWMA should be selected + } public void testSerialization() throws Exception { @@ -51,15 +60,15 @@ public void testSerialization() throws Exception { BytesStreamOutput os = new BytesStreamOutput(); Map testMap = new HashMap<>(); - testMap.put(TierType.ON_HEAP, new StatsHolder(11, 12, 13, 14, 15)); - testMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10)); + testMap.put(TierType.ON_HEAP, new StatsHolder(11, 12, 13, 14, 15, 0.0)); + testMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10, 25.0)); RequestCacheStats stats = new RequestCacheStats(testMap); stats.writeTo(os); BytesStreamInput is = new BytesStreamInput(BytesReference.toBytes(os.bytes())); RequestCacheStats deserialized = new RequestCacheStats(is); - assertTierState(deserialized, TierType.ON_HEAP, 11, 12, 13, 14, 15); - assertTierState(deserialized, TierType.DISK, 6, 7, 8, 9, 10); + assertTierState(deserialized, TierType.ON_HEAP, 11, 12, 13, 14, 15, 0.0); + assertTierState(deserialized, TierType.DISK, 6, 7, 8, 9, 10, 25.0); } private void assertTierState( @@ -69,12 +78,14 @@ private void assertTierState( long evictions, long hitCount, long missCount, - long entries + long entries, + double getTimeEWMA ) { assertEquals(memSize, stats.getMemorySizeInBytes(tierType)); assertEquals(evictions, stats.getEvictions(tierType)); assertEquals(hitCount, stats.getHitCount(tierType)); assertEquals(missCount, stats.getMissCount(tierType)); assertEquals(entries, stats.getEntries(tierType)); + assertEquals(getTimeEWMA, stats.getTimeEWMA(tierType), 0.01); } } diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 5df6b2154c4d8..31ff072a7ecba 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -180,7 +180,6 @@ public void testSpillover() throws Exception { TestEntity entity = new TestEntity(requestCacheStats, indexShard); Loader loader = new Loader(reader, 0); - System.out.println("On-heap cache size at start = " + requestCacheStats.stats().getMemorySizeInBytes()); BytesReference[] termBytesArr = new BytesReference[maxNumInHeap + 1]; for (int i = 0; i < maxNumInHeap + 1; i++) { diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java index 766d80a81b097..0b20bd07d647b 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java @@ -341,10 +341,10 @@ public Object getCacheIdentity() { } @Override - public void onHit(TierType tierType) {} + public void onHit(TierType tierType, double getTimeEWMA) {} @Override - public void onMiss(TierType tierType) {} + public void onMiss(TierType tierType, double getTimeEWMA) {} @Override public void onRemoval(RemovalNotification notification) {}