From fd9af050eb62a99563af4da3a7b6ef09f9927581 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 18 Oct 2023 14:37:07 -0700 Subject: [PATCH 01/18] added test entries metric for on heap --- .../cache/request/RequestCacheStats.java | 13 +++- .../cache/request/ShardRequestCache.java | 76 +++++++++++++++---- 2 files changed, 74 insertions(+), 15 deletions(-) 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 24f68899c2ac7..5aa9f57b64032 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,6 +52,7 @@ public class RequestCacheStats implements Writeable, ToXContentFragment { private long evictions; private long hitCount; private long missCount; + private long entries; // number of entries in the cache public RequestCacheStats() {} @@ -60,13 +61,15 @@ public RequestCacheStats(StreamInput in) throws IOException { evictions = in.readVLong(); hitCount = in.readVLong(); missCount = in.readVLong(); + entries = in.readVLong(); } - public RequestCacheStats(long memorySize, long evictions, long hitCount, long missCount) { + public RequestCacheStats(long memorySize, long evictions, long hitCount, long missCount, long entries) { // this.memorySize = memorySize; this.evictions = evictions; this.hitCount = hitCount; this.missCount = missCount; + this.entries = entries; } public void add(RequestCacheStats stats) { @@ -74,6 +77,7 @@ public void add(RequestCacheStats stats) { this.evictions += stats.evictions; this.hitCount += stats.hitCount; this.missCount += stats.missCount; + this.entries += stats.entries; } public long getMemorySizeInBytes() { @@ -96,12 +100,17 @@ public long getMissCount() { return this.missCount; } + public long getEntries() { + return this.entries; + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeVLong(memorySize); out.writeVLong(evictions); out.writeVLong(hitCount); out.writeVLong(missCount); + out.writeVLong(entries); } @Override @@ -111,6 +120,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(Fields.EVICTIONS, getEvictions()); builder.field(Fields.HIT_COUNT, getHitCount()); builder.field(Fields.MISS_COUNT, getMissCount()); + builder.field(Fields.ENTRIES, getEntries()); builder.endObject(); return builder; } @@ -127,5 +137,6 @@ static final class Fields { static final String EVICTIONS = "evictions"; static final String HIT_COUNT = "hit_count"; static final String MISS_COUNT = "miss_count"; + static final String ENTRIES = "entries"; } } 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 795d585d88647..e2c933b7e0f93 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 @@ -33,9 +33,13 @@ package org.opensearch.index.cache.request; import org.apache.lucene.util.Accountable; +import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.core.common.bytes.BytesReference; +import java.io.Serializable; +import java.util.EnumMap; + /** * Tracks the portion of the request cache in use for a particular shard. * @@ -43,31 +47,65 @@ */ public final class ShardRequestCache { - final CounterMetric evictionsMetric = new CounterMetric(); - final CounterMetric totalMetric = new CounterMetric(); - final CounterMetric hitCount = new CounterMetric(); - final CounterMetric missCount = new CounterMetric(); + // Holds stats common to all tiers + private final EnumMap statsHolder = new EnumMap<>(TierType.class); + + public ShardRequestCache() { + for (TierType tierType : TierType.values()) { + statsHolder.put(tierType, new StatsHolder()); + } + } public RequestCacheStats stats() { // TODO: Change RequestCacheStats to support disk tier stats. - return new RequestCacheStats(totalMetric.count(), evictionsMetric.count(), hitCount.count(), missCount.count()); + return stats(TierType.ON_HEAP); //? } - public void onHit() { - hitCount.inc(); + public RequestCacheStats stats(TierType tierType) { + return new RequestCacheStats( + statsHolder.get(tierType).totalMetric.count(), + statsHolder.get(tierType).evictionsMetric.count(), + statsHolder.get(tierType).hitCount.count(), + statsHolder.get(tierType).missCount.count(), + statsHolder.get(tierType).entries.count() + ); } - public void onMiss() { - missCount.inc(); + public RequestCacheStats overallStats() { + long totalSize = 0; + long totalEvictions = 0; + long totalHits = 0; + long totalMisses = 0; + long totalEntries = 0; + for (TierType tierType : TierType.values()) { + totalSize += statsHolder.get(tierType).totalMetric.count(); + totalEvictions += statsHolder.get(tierType).evictionsMetric.count(); + totalHits += statsHolder.get(tierType).hitCount.count(); + totalMisses += statsHolder.get(tierType).missCount.count(); + totalEntries += statsHolder.get(tierType).entries.count(); + } + return new RequestCacheStats( + totalSize, + totalEvictions, + totalHits, + totalMisses, + totalEntries + ); } - public void onCached(Accountable key, BytesReference value) { - totalMetric.inc(key.ramBytesUsed() + value.ramBytesUsed()); + public void onMiss(TierType tierType) { + statsHolder.get(tierType).missCount.inc(); } - public void onRemoval(Accountable key, BytesReference value, boolean evicted) { + public void onCached(Accountable key, BytesReference value, TierType tierType) { + statsHolder.get(tierType).totalMetric.inc(key.ramBytesUsed() + value.ramBytesUsed()); + statsHolder.get(tierType).entries.inc(); + } + + public void onRemoval(Accountable key, BytesReference value, boolean evicted, TierType tierType) { + if (evicted) { - evictionsMetric.inc(); + statsHolder.get(tierType).evictionsMetric.inc(); } long dec = 0; if (key != null) { @@ -76,6 +114,16 @@ public void onRemoval(Accountable key, BytesReference value, boolean evicted) { if (value != null) { dec += value.ramBytesUsed(); } - totalMetric.dec(dec); + statsHolder.get(tierType).totalMetric.dec(dec); + statsHolder.get(tierType).entries.dec(); + } + + static class StatsHolder implements Serializable { + + final CounterMetric evictionsMetric = new CounterMetric(); + final CounterMetric totalMetric = new CounterMetric(); + final CounterMetric hitCount = new CounterMetric(); + final CounterMetric missCount = new CounterMetric(); + final CounterMetric entries = new CounterMetric(); } } From 0fe6bf883f38a8908e23c48043a79804b61677fe Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 18 Oct 2023 16:07:03 -0700 Subject: [PATCH 02/18] Added check for new entries field in IT test, added permissions allowing the domain to run --- .../opensearch/indices/IndicesRequestCacheIT.java | 15 ++++++++++++++- .../org/opensearch/bootstrap/security.policy | 8 +++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 5dcd5a9f44c7a..c8363c6ab4019 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -664,6 +664,7 @@ public void testCacheWithInvalidation() throws Exception { assertSearchResponse(resp); // Should expect hit as here as refresh didn't happen assertCacheState(client, "index", 1, 1); + assertNumCacheEntries(client, "index", 1); // Explicit refresh would invalidate cache refresh(); @@ -672,6 +673,8 @@ public void testCacheWithInvalidation() throws Exception { assertSearchResponse(resp); // Should expect miss as key has changed due to change in IndexReader.CacheKey (due to refresh) assertCacheState(client, "index", 1, 2); + assertNumCacheEntries(client, "index", 1); // Shouldn't it just be the most recent query, since the first one was invalidated? (prob invalidation isnt in yet) + // yeah - evictions = 0, its not in yet } private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { @@ -690,5 +693,15 @@ private static void assertCacheState(Client client, String index, long expectedH ); } - + + private static void assertNumCacheEntries(Client client, String index, long expectedEntries) { + RequestCacheStats requestCacheStats = client.admin() + .indices() + .prepareStats(index) + .setRequestCache(true) + .get() + .getTotal() + .getRequestCache(); + assertEquals(expectedEntries, requestCacheStats.getEntries()); + } } diff --git a/server/src/main/resources/org/opensearch/bootstrap/security.policy b/server/src/main/resources/org/opensearch/bootstrap/security.policy index 5d588bb9bf1fd..a509ecb544546 100644 --- a/server/src/main/resources/org/opensearch/bootstrap/security.policy +++ b/server/src/main/resources/org/opensearch/bootstrap/security.policy @@ -191,7 +191,13 @@ grant { // For ehcache permission java.lang.RuntimePermission "createClassLoader"; permission java.lang.RuntimePermission "accessClassInPackage.sun.misc"; - + permission java.lang.RuntimePermission "getenv.*"; permission java.lang.RuntimePermission "accessDeclaredMembers"; permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; + permission java.io.FilePermission "disk_cache_tier", "read"; // change this to wherever we will put disk tier folder + permission java.io.FilePermission "disk_cache_tier", "write"; + permission java.io.FilePermission "disk_cache_tier", "delete"; + permission java.io.FilePermission "disk_cache_tier/-", "read"; + permission java.io.FilePermission "disk_cache_tier/-", "write"; + permission java.io.FilePermission "disk_cache_tier/-", "delete"; }; From 93bfe472652ef81a2fb42c7f4949d14c17fa6c86 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 20 Oct 2023 15:51:38 -0700 Subject: [PATCH 03/18] Initial implementation for tiered node request cache stats --- .../common/cache/tier/TierType.java | 15 ++- .../cache/request/RequestCacheStats.java | 120 ++++++++++++------ .../cache/request/ShardRequestCache.java | 45 +------ .../index/cache/request/StatsHolder.java | 99 +++++++++++++++ .../indices/IndicesRequestCacheTests.java | 120 ++++++++++++++++++ 5 files changed, 316 insertions(+), 83 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TierType.java b/server/src/main/java/org/opensearch/common/cache/tier/TierType.java index ca61b636c1dda..3ef4338030899 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/TierType.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/TierType.java @@ -13,6 +13,17 @@ */ public enum TierType { - ON_HEAP, - DISK; + ON_HEAP("on_heap"), + DISK("disk"); + + private final String stringValue; + + TierType(String stringValue) { + // Associate each TierType with a string representation, for use in API responses and elsewhere + this.stringValue = stringValue; + } + + public String getStringValue() { + return this.stringValue; + } } 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 5aa9f57b64032..65bcc90634016 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 @@ -38,8 +38,11 @@ import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.common.cache.tier.TierType; import java.io.IOException; +import java.util.HashMap; +import java.util.Map; /** * Request for the query cache statistics @@ -48,79 +51,117 @@ */ public class RequestCacheStats implements Writeable, ToXContentFragment { - private long memorySize; - private long evictions; - private long hitCount; - private long missCount; - private long entries; // number of entries in the cache - - public RequestCacheStats() {} + private Map map; + public RequestCacheStats() { + this.map = new HashMap<>(); + for (TierType tierType : TierType.values()) { + map.put(tierType.getStringValue(), new StatsHolder()); + // Every possible tier type must have counters, even if they are disabled. Then the counters report 0 + } + } public RequestCacheStats(StreamInput in) throws IOException { - memorySize = in.readVLong(); - evictions = in.readVLong(); - hitCount = in.readVLong(); - missCount = in.readVLong(); - entries = in.readVLong(); + // Any RequestCacheStats written to a stream should already have a counter for each possible tier type + this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need to be registered? + } + + public RequestCacheStats(TierType tierType, StatsHolder statsHolder) { + // Create a RequestCacheStats object with only one tier's statistics populated + this(); + map.put(tierType.getStringValue(), statsHolder); } - public RequestCacheStats(long memorySize, long evictions, long hitCount, long missCount, long entries) { // - this.memorySize = memorySize; - this.evictions = evictions; - this.hitCount = hitCount; - this.missCount = missCount; - this.entries = entries; + public RequestCacheStats(Map inputMap) { + // Create a RequestCacheStats with multiple tiers' statistics + this(); + for (TierType tierType : inputMap.keySet()) { + map.put(tierType.getStringValue(), inputMap.get(tierType)); + } } + // can prob eliminate some of these constructors + public void add(RequestCacheStats stats) { - this.memorySize += stats.memorySize; - this.evictions += stats.evictions; - this.hitCount += stats.hitCount; - this.missCount += stats.missCount; - this.entries += stats.entries; + for (String tier : stats.map.keySet()) { + map.get(tier).add(stats.map.get(tier)); + } + } + + private StatsHolder getTierStats(TierType tierType) { + return map.get(tierType.getStringValue()); + } + + // should these take in strings bc thats whats done in the xcontent builder? seems wasteful + public long getMemorySizeInBytes(TierType tierType) { + return getTierStats(tierType).totalMetric.count(); + } + + public ByteSizeValue getMemorySize(TierType tierType) { + return new ByteSizeValue(getMemorySizeInBytes(tierType)); } + public long getEvictions(TierType tierType) { + return getTierStats(tierType).evictionsMetric.count(); + } + + public long getHitCount(TierType tierType) { + return getTierStats(tierType).hitCount.count(); + } + + public long getMissCount(TierType tierType) { + return getTierStats(tierType).missCount.count(); + } + + public long getEntries(TierType tierType) { + return getTierStats(tierType).entries.count(); + } + + // By default, return on-heap stats if no tier is specified + public long getMemorySizeInBytes() { - return this.memorySize; + return getMemorySizeInBytes(TierType.ON_HEAP); } public ByteSizeValue getMemorySize() { - return new ByteSizeValue(memorySize); + return getMemorySize(TierType.ON_HEAP); } public long getEvictions() { - return this.evictions; + return getEvictions(TierType.ON_HEAP); } public long getHitCount() { - return this.hitCount; + return getHitCount(TierType.ON_HEAP); } public long getMissCount() { - return this.missCount; + return getMissCount(TierType.ON_HEAP); } public long getEntries() { - return this.entries; + return getEntries(TierType.ON_HEAP); } @Override public void writeTo(StreamOutput out) throws IOException { - out.writeVLong(memorySize); - out.writeVLong(evictions); - out.writeVLong(hitCount); - out.writeVLong(missCount); - out.writeVLong(entries); + out.writeMap(this.map, StreamOutput::writeString, (o, v) -> v.writeTo(o)); // ? } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(Fields.REQUEST_CACHE_STATS); - builder.humanReadableField(Fields.MEMORY_SIZE_IN_BYTES, Fields.MEMORY_SIZE, getMemorySize()); - builder.field(Fields.EVICTIONS, getEvictions()); - builder.field(Fields.HIT_COUNT, getHitCount()); - builder.field(Fields.MISS_COUNT, getMissCount()); - builder.field(Fields.ENTRIES, getEntries()); + // write on-heap stats outside of tiers object + getTierStats(TierType.ON_HEAP).toXContent(builder, params); + 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); + builder.endObject(); + } + } + builder.endObject(); builder.endObject(); return builder; } @@ -132,6 +173,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws */ static final class Fields { static final String REQUEST_CACHE_STATS = "request_cache"; + static final String TIERS = "tiers"; static final String MEMORY_SIZE = "memory_size"; static final String MEMORY_SIZE_IN_BYTES = "memory_size_in_bytes"; static final String EVICTIONS = "evictions"; 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 e2c933b7e0f93..23e3b141bdd37 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 @@ -37,7 +37,6 @@ import org.opensearch.common.metrics.CounterMetric; import org.opensearch.core.common.bytes.BytesReference; -import java.io.Serializable; import java.util.EnumMap; /** @@ -58,40 +57,11 @@ public ShardRequestCache() { public RequestCacheStats stats() { // TODO: Change RequestCacheStats to support disk tier stats. - return stats(TierType.ON_HEAP); //? + // Changing this function to return a RequestCacheStats with stats from all tiers. + //return stats(TierType.ON_HEAP); + return new RequestCacheStats(statsHolder); } - public RequestCacheStats stats(TierType tierType) { - return new RequestCacheStats( - statsHolder.get(tierType).totalMetric.count(), - statsHolder.get(tierType).evictionsMetric.count(), - statsHolder.get(tierType).hitCount.count(), - statsHolder.get(tierType).missCount.count(), - statsHolder.get(tierType).entries.count() - ); - } - - public RequestCacheStats overallStats() { - long totalSize = 0; - long totalEvictions = 0; - long totalHits = 0; - long totalMisses = 0; - long totalEntries = 0; - for (TierType tierType : TierType.values()) { - totalSize += statsHolder.get(tierType).totalMetric.count(); - totalEvictions += statsHolder.get(tierType).evictionsMetric.count(); - totalHits += statsHolder.get(tierType).hitCount.count(); - totalMisses += statsHolder.get(tierType).missCount.count(); - totalEntries += statsHolder.get(tierType).entries.count(); - } - return new RequestCacheStats( - totalSize, - totalEvictions, - totalHits, - totalMisses, - totalEntries - ); - } public void onMiss(TierType tierType) { statsHolder.get(tierType).missCount.inc(); @@ -117,13 +87,4 @@ public void onRemoval(Accountable key, BytesReference value, boolean evicted, Ti statsHolder.get(tierType).totalMetric.dec(dec); statsHolder.get(tierType).entries.dec(); } - - static class StatsHolder implements Serializable { - - final CounterMetric evictionsMetric = new CounterMetric(); - final CounterMetric totalMetric = new CounterMetric(); - final CounterMetric hitCount = new CounterMetric(); - final CounterMetric missCount = new CounterMetric(); - final CounterMetric entries = new CounterMetric(); - } } 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 new file mode 100644 index 0000000000000..86f99871fea26 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/cache/request/StatsHolder.java @@ -0,0 +1,99 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.cache.request; + +import org.opensearch.common.metrics.CounterMetric; +import org.opensearch.core.common.io.stream.StreamInput; +import org.opensearch.core.common.io.stream.StreamOutput; +import org.opensearch.core.common.io.stream.Writeable; +import org.opensearch.core.common.unit.ByteSizeValue; +import org.opensearch.core.xcontent.ToXContentFragment; +import org.opensearch.core.xcontent.XContentBuilder; + +import java.io.IOException; +import java.io.Serializable; + +public class StatsHolder implements Serializable, Writeable, ToXContentFragment { + final CounterMetric evictionsMetric; + final CounterMetric totalMetric; + final CounterMetric hitCount; + final CounterMetric missCount; + final CounterMetric entries; + + + public StatsHolder() { + this.evictionsMetric = new CounterMetric(); + this.totalMetric = new CounterMetric(); + this.hitCount = new CounterMetric(); + this.missCount = new CounterMetric(); + this.entries = new CounterMetric(); + } + + public StatsHolder(StreamInput in) throws IOException { + // Read and write the values of the counter metrics. They should always be positive + this.evictionsMetric = new CounterMetric(); + this.evictionsMetric.inc(in.readVLong()); + this.totalMetric = new CounterMetric(); + this.totalMetric.inc(in.readVLong()); + this.hitCount = new CounterMetric(); + this.hitCount.inc(in.readVLong()); + this.missCount = new CounterMetric(); + this.missCount.inc(in.readVLong()); + this.entries = new CounterMetric(); + this.entries.inc(in.readVLong()); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVLong(evictionsMetric.count()); + out.writeVLong(totalMetric.count()); + out.writeVLong(hitCount.count()); + out.writeVLong(missCount.count()); + out.writeVLong(entries.count()); + } + + public void add(StatsHolder otherStats) { + // Add the argument's metrics to this object's metrics. + evictionsMetric.inc(otherStats.evictionsMetric.count()); + totalMetric.inc(otherStats.totalMetric.count()); + hitCount.inc(otherStats.hitCount.count()); + missCount.inc(otherStats.missCount.count()); + entries.inc(otherStats.entries.count()); + } + + public long getEvictions() { + return evictionsMetric.count(); + } + + public long getMemorySize() { + return totalMetric.count(); + } + + public long getHitCount() { + return hitCount.count(); + } + + public long getMissCount() { + return missCount.count(); + } + + public long getEntries() { + return entries.count(); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.humanReadableField(RequestCacheStats.Fields.MEMORY_SIZE_IN_BYTES, RequestCacheStats.Fields.MEMORY_SIZE, new ByteSizeValue(getMemorySize())); + builder.field(RequestCacheStats.Fields.EVICTIONS, getEvictions()); + builder.field(RequestCacheStats.Fields.HIT_COUNT, getHitCount()); + builder.field(RequestCacheStats.Fields.MISS_COUNT, getMissCount()); + builder.field(RequestCacheStats.Fields.ENTRIES, getEntries()); + return builder; + } +} diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 5fbffe6906d56..6e5fcadf53a2e 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -124,6 +124,126 @@ public void testBasicOperationsCache() throws Exception { IOUtils.close(reader, writer, dir, cache); assertEquals(0, cache.numRegisteredCloseListeners()); + cache.closeDiskTier(); + } + + public void testAddDirectToEhcache() throws Exception { + // this test is for debugging serialization issues and can eventually be removed + // Put breakpoints at line 260 of AbstractOffHeapStore to catch serialization errors + // that would otherwise fail silently + ShardRequestCache requestCacheStats = new ShardRequestCache(); + Settings.Builder settingsBuilder = Settings.builder(); + long heapSizeBytes = 1000; + settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); + IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); + + // set up a key + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + AtomicBoolean indexShard = new AtomicBoolean(true); + TestEntity entity = new TestEntity(requestCacheStats, indexShard); + TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + String rKey = ((OpenSearchDirectoryReader) reader).getDelegatingCacheHelper().getDelegatingCacheKey().getId().toString(); + IndicesRequestCache.Key key = cache.new Key(entity, termBytes, rKey); + + BytesReference value = new BytesArray(new byte[]{0}); + cache.tieredCacheHandler.getDiskCachingTier().put(key, value); + + BytesReference res = cache.tieredCacheHandler.getDiskCachingTier().get(key); + assertEquals(value, res); + assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); + + IOUtils.close(reader, writer, dir, cache); + cache.closeDiskTier(); + } + + public void testSpillover() throws Exception { + // fill the on-heap cache until we spill over + ShardRequestCache requestCacheStats = new ShardRequestCache(); + Settings.Builder settingsBuilder = Settings.builder(); + long heapSizeBytes = 1000; // each of these queries is 131 bytes, so we can fit 7 in the heap cache + int heapKeySize = 131; + int maxNumInHeap = 1000 / heapKeySize; + settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); + IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); + + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + AtomicBoolean indexShard = new AtomicBoolean(true); + + 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++) { + TermQueryBuilder termQuery = new TermQueryBuilder("id", String.valueOf(i)); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + String rKey = ((OpenSearchDirectoryReader) reader).getDelegatingCacheHelper().getDelegatingCacheKey().getId().toString(); + termBytesArr[i] = termBytes; + BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + } + // get value from disk cache, the first key should have been evicted + BytesReference firstValue = cache.getOrCompute(entity, loader, reader, termBytesArr[0]); + + assertEquals(maxNumInHeap * heapKeySize, requestCacheStats.stats().getMemorySizeInBytes()); + // TODO: disk weight bytes + assertEquals(1, requestCacheStats.stats().getEvictions()); + assertEquals(1, requestCacheStats.stats().getHitCount(TierType.DISK)); + assertEquals(maxNumInHeap + 1, requestCacheStats.stats().getMissCount(TierType.DISK)); + assertEquals(0, requestCacheStats.stats().getHitCount()); + assertEquals(maxNumInHeap + 2, requestCacheStats.stats().getMissCount()); + assertEquals(maxNumInHeap, cache.tieredCacheHandler.count(TierType.ON_HEAP)); + assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); + + // get a value from heap cache, second key should still be there + BytesReference secondValue = cache.getOrCompute(entity, loader, reader, termBytesArr[1]); + // get the value on disk cache again + BytesReference firstValueAgain = cache.getOrCompute(entity, loader, reader, termBytesArr[0]); + + assertEquals(1, requestCacheStats.stats().getEvictions()); + assertEquals(2, requestCacheStats.stats().getHitCount(TierType.DISK)); + assertEquals(maxNumInHeap + 1, requestCacheStats.stats().getMissCount(TierType.DISK)); + assertEquals(1, requestCacheStats.stats().getHitCount()); + assertEquals(maxNumInHeap + 3, requestCacheStats.stats().getMissCount()); + assertEquals(maxNumInHeap, cache.tieredCacheHandler.count(TierType.ON_HEAP)); + assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); + + IOUtils.close(reader, writer, dir, cache); + cache.closeDiskTier(); + } + + public void testDiskGetTimeEWMA() throws Exception { + ShardRequestCache requestCacheStats = new ShardRequestCache(); + Settings.Builder settingsBuilder = Settings.builder(); + long heapSizeBytes = 0; // skip directly to disk cache + settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); + IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); + + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + AtomicBoolean indexShard = new AtomicBoolean(true); + + TestEntity entity = new TestEntity(requestCacheStats, indexShard); + Loader loader = new Loader(reader, 0); + + for (int i = 0; i < 50; i++) { + TermQueryBuilder termQuery = new TermQueryBuilder("id", String.valueOf(i)); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + // on my machine get time EWMA converges to ~0.025 ms, but it does have an SSD + assertTrue(cache.tieredCacheHandler.diskGetTimeMillisEWMA() > 0); + } + + IOUtils.close(reader, writer, dir, cache); + cache.closeDiskTier(); } public void testCacheDifferentReaders() throws Exception { From 93f75938df2812dd657bdb22c1cfca66bf39b8a7 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 20 Oct 2023 16:47:20 -0700 Subject: [PATCH 04/18] Added version checks to streaming functions --- .../cache/request/RequestCacheStats.java | 38 ++++++++++++++----- .../index/cache/request/StatsHolder.java | 21 ++++++---- 2 files changed, 43 insertions(+), 16 deletions(-) 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 65bcc90634016..42f7cfa15d3a6 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 @@ -32,6 +32,7 @@ package org.opensearch.index.cache.request; +import org.opensearch.Version; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; @@ -61,14 +62,25 @@ public RequestCacheStats() { } public RequestCacheStats(StreamInput in) throws IOException { - // Any RequestCacheStats written to a stream should already have a counter for each possible tier type - this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need to be registered? - } - - public RequestCacheStats(TierType tierType, StatsHolder statsHolder) { - // Create a RequestCacheStats object with only one tier's statistics populated this(); - map.put(tierType.getStringValue(), statsHolder); + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need to be registered? + } else { + // objects from earlier versions only contain on-heap info, and do not have entries 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 + )); + } } public RequestCacheStats(Map inputMap) { @@ -91,7 +103,6 @@ private StatsHolder getTierStats(TierType tierType) { return map.get(tierType.getStringValue()); } - // should these take in strings bc thats whats done in the xcontent builder? seems wasteful public long getMemorySizeInBytes(TierType tierType) { return getTierStats(tierType).totalMetric.count(); } @@ -144,7 +155,16 @@ public long getEntries() { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeMap(this.map, StreamOutput::writeString, (o, v) -> v.writeTo(o)); // ? + 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 + StatsHolder heapStats = map.get(TierType.ON_HEAP.getStringValue()); + out.writeVLong(heapStats.getMemorySize()); + out.writeVLong(heapStats.getEvictions()); + out.writeVLong(heapStats.getHitCount()); + out.writeVLong(heapStats.getMissCount()); + } } @Override 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 86f99871fea26..d506d8e78ec7a 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 @@ -35,18 +35,25 @@ public StatsHolder() { this.entries = new CounterMetric(); } - public StatsHolder(StreamInput in) throws IOException { - // Read and write the values of the counter metrics. They should always be positive + public StatsHolder(long evictions, long memorySize, long hitCount, long missCount, long entries) { this.evictionsMetric = new CounterMetric(); - this.evictionsMetric.inc(in.readVLong()); + this.evictionsMetric.inc(evictions); this.totalMetric = new CounterMetric(); - this.totalMetric.inc(in.readVLong()); + this.totalMetric.inc(memorySize); this.hitCount = new CounterMetric(); - this.hitCount.inc(in.readVLong()); + this.hitCount.inc(hitCount); this.missCount = new CounterMetric(); - this.missCount.inc(in.readVLong()); + this.missCount.inc(missCount); this.entries = new CounterMetric(); - this.entries.inc(in.readVLong()); + this.entries.inc(entries); + } + + 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()); + // 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) } @Override From 46d1e906fa37c5f7548a188b763178f510e4c5b2 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 23 Oct 2023 12:37:29 -0700 Subject: [PATCH 05/18] Added UT for RequestCacheStats --- .../indices/IndicesRequestCacheIT.java | 43 ++++++--- .../index/cache/request/StatsHolder.java | 15 +-- .../cache/request/RequestCacheStatsTests.java | 95 +++++++++++++++++++ 3 files changed, 134 insertions(+), 19 deletions(-) create mode 100644 server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index c8363c6ab4019..246bb7c57c07c 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -34,6 +34,7 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; +import org.opensearch.action.IndicesRequestIT; import org.opensearch.action.admin.indices.alias.Alias; import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; import org.opensearch.action.search.SearchResponse; @@ -44,6 +45,7 @@ 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; @@ -637,16 +639,27 @@ public void testProfileDisableCache() throws Exception { public void testCacheWithInvalidation() throws Exception { Client client = client(); + //int heapSizeBytes = 2000; // enough to fit 2 queries, as each is 687 B + + Settings.Builder builder = Settings.builder() + .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + .put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(2000)); + // Why is it appending "index." to the beginning of the key?? + + String heapSizeBytes = builder.get(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey()); + System.out.println("Current cap = " + heapSizeBytes); + + client.admin().setSettings(Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(2000))); + assertAcked( client.admin() .indices() .prepareCreate("index") .setMapping("k", "type=keyword") .setSettings( - Settings.builder() - .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + builder ) .get() ); @@ -663,8 +676,9 @@ public void testCacheWithInvalidation() throws Exception { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); assertSearchResponse(resp); // Should expect hit as here as refresh didn't happen - assertCacheState(client, "index", 1, 1); - assertNumCacheEntries(client, "index", 1); + assertCacheState(client, "index", 1, 1, TierType.ON_HEAP); + assertCacheState(client, "index", 0, 1, TierType.DISK); + assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Explicit refresh would invalidate cache refresh(); @@ -672,12 +686,13 @@ public void testCacheWithInvalidation() throws Exception { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); assertSearchResponse(resp); // Should expect miss as key has changed due to change in IndexReader.CacheKey (due to refresh) - assertCacheState(client, "index", 1, 2); - assertNumCacheEntries(client, "index", 1); // Shouldn't it just be the most recent query, since the first one was invalidated? (prob invalidation isnt in yet) + assertCacheState(client, "index", 1, 2, TierType.ON_HEAP); + assertCacheState(client, "index", 0, 2, TierType.DISK); + assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Shouldn't it just be the most recent query, since the first one was invalidated? (prob invalidation isnt in yet) // yeah - evictions = 0, its not in yet } - private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { + private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses, TierType tierType) { RequestCacheStats requestCacheStats = client.admin() .indices() .prepareStats(index) @@ -687,14 +702,18 @@ private static void assertCacheState(Client client, String index, long expectedH .getRequestCache(); // Check the hit count and miss count together so if they are not // correct we can see both values + System.out.println("mem size " + requestCacheStats.getMemorySize(tierType)); assertEquals( Arrays.asList(expectedHits, expectedMisses, 0L), - Arrays.asList(requestCacheStats.getHitCount(), requestCacheStats.getMissCount(), requestCacheStats.getEvictions()) + Arrays.asList(requestCacheStats.getHitCount(tierType), requestCacheStats.getMissCount(tierType), requestCacheStats.getEvictions(tierType)) ); + } + private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { + assertCacheState(client, index, expectedHits, expectedMisses, TierType.ON_HEAP); } - private static void assertNumCacheEntries(Client client, String index, long expectedEntries) { + private static void assertNumCacheEntries(Client client, String index, long expectedEntries, TierType tierType) { RequestCacheStats requestCacheStats = client.admin() .indices() .prepareStats(index) @@ -702,6 +721,6 @@ private static void assertNumCacheEntries(Client client, String index, long expe .get() .getTotal() .getRequestCache(); - assertEquals(expectedEntries, requestCacheStats.getEntries()); + assertEquals(expectedEntries, requestCacheStats.getEntries(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 d506d8e78ec7a..c7d7447a68459 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 @@ -20,26 +20,27 @@ import java.io.Serializable; public class StatsHolder implements Serializable, Writeable, ToXContentFragment { - final CounterMetric evictionsMetric; final CounterMetric totalMetric; + final CounterMetric evictionsMetric; final CounterMetric hitCount; final CounterMetric missCount; final CounterMetric entries; public StatsHolder() { - this.evictionsMetric = new CounterMetric(); this.totalMetric = new CounterMetric(); + this.evictionsMetric = new CounterMetric(); this.hitCount = new CounterMetric(); this.missCount = new CounterMetric(); this.entries = new CounterMetric(); } - public StatsHolder(long evictions, long memorySize, long hitCount, long missCount, long entries) { - this.evictionsMetric = new CounterMetric(); - this.evictionsMetric.inc(evictions); + public StatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries) { + // Switched argument order to match RequestCacheStats this.totalMetric = new CounterMetric(); this.totalMetric.inc(memorySize); + this.evictionsMetric = new CounterMetric(); + this.evictionsMetric.inc(evictions); this.hitCount = new CounterMetric(); this.hitCount.inc(hitCount); this.missCount = new CounterMetric(); @@ -58,8 +59,8 @@ public StatsHolder(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeVLong(evictionsMetric.count()); out.writeVLong(totalMetric.count()); + out.writeVLong(evictionsMetric.count()); out.writeVLong(hitCount.count()); out.writeVLong(missCount.count()); out.writeVLong(entries.count()); @@ -67,8 +68,8 @@ public void writeTo(StreamOutput out) throws IOException { public void add(StatsHolder otherStats) { // Add the argument's metrics to this object's metrics. - evictionsMetric.inc(otherStats.evictionsMetric.count()); totalMetric.inc(otherStats.totalMetric.count()); + evictionsMetric.inc(otherStats.evictionsMetric.count()); hitCount.inc(otherStats.hitCount.count()); missCount.inc(otherStats.missCount.count()); entries.inc(otherStats.entries.count()); 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 new file mode 100644 index 0000000000000..0b1adbe0f87c7 --- /dev/null +++ b/server/src/test/java/org/opensearch/index/cache/request/RequestCacheStatsTests.java @@ -0,0 +1,95 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.cache.request; + +import org.opensearch.common.io.stream.BytesStreamOutput; +import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.common.io.stream.BytesStream; +import org.opensearch.core.common.io.stream.BytesStreamInput; +import org.opensearch.core.common.io.stream.StreamInput; +import org.opensearch.core.common.io.stream.StreamOutput; +import org.opensearch.indices.TierType; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +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); + } + Map testHeapMap = new HashMap<>(); + testHeapMap.put(TierType.ON_HEAP, new StatsHolder( + 1, 2, 3, 4, 5 + )); + RequestCacheStats heapOnlyStats = new RequestCacheStats(testHeapMap); + for (TierType tierType : TierType.values()) { + if (tierType == TierType.ON_HEAP) { + assertTierState(heapOnlyStats, tierType, 1, 2, 3, 4, 5); + } else { + assertTierState(heapOnlyStats, tierType, 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 + )); + RequestCacheStats bothTiersStats = new RequestCacheStats(testBothTiersMap); + assertTierState(bothTiersStats, TierType.ON_HEAP, 11, 12, 13, 14, 15); + assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); + + bothTiersStats.add(heapOnlyStats); + assertTierState(bothTiersStats, TierType.ON_HEAP, 12, 14, 16, 18, 20); + assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); + } + + public void testSerialization() throws Exception { + // This test also implicitly tests StreamHolder serialization + 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 + )); + 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); + } + + private void assertTierState( + RequestCacheStats stats, + TierType tierType, + long memSize, + long evictions, + long hitCount, + long missCount, + long entries + ) { + 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)); + } + +} From eca9172dc05f32bfce83406661855b3f93babf4b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 23 Oct 2023 15:20:14 -0700 Subject: [PATCH 06/18] Added IT for disk tier stats, plus spotlessApply --- .../IndicesRequestCacheDiskTierIT.java | 88 +++++++++++++++++++ .../indices/IndicesRequestCacheIT.java | 72 ++++++++------- .../cache/request/RequestCacheStats.java | 14 +-- .../cache/request/ShardRequestCache.java | 5 +- .../index/cache/request/StatsHolder.java | 9 +- .../indices/IndicesRequestCache.java | 1 - .../cache/request/RequestCacheStatsTests.java | 27 ++---- .../indices/IndicesRequestCacheTests.java | 2 +- 8 files changed, 147 insertions(+), 71 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java new file mode 100644 index 0000000000000..47bda50ca788a --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -0,0 +1,88 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.indices; + +import org.opensearch.action.search.SearchResponse; +import org.opensearch.client.Client; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.settings.Settings; +import org.opensearch.core.common.unit.ByteSizeValue; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.test.OpenSearchIntegTestCase; + +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; + +// This is a separate file from IndicesRequestCacheIT because we only want to run our test +// on a node with a maximum request cache size that we set. + +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) +public class IndicesRequestCacheDiskTierIT extends OpenSearchIntegTestCase { + public void testDiskTierStats() throws Exception { + int heapSizeBytes = 1800; // enough to fit 2 queries, as each is 687 B + int requestSize = 687; // each request is 687 B + String node = internalCluster().startNode( + Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(heapSizeBytes)) + ); + Client client = client(node); + + Settings.Builder indicesSettingBuilder = Settings.builder() + .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0); + + assertAcked( + client.admin().indices().prepareCreate("index").setMapping("k", "type=keyword").setSettings(indicesSettingBuilder).get() + ); + indexRandom(true, client.prepareIndex("index").setSource("k", "hello")); + ensureSearchable("index"); + SearchResponse resp; + + int numOnDisk = 5; + int numRequests = heapSizeBytes / requestSize + numOnDisk; + for (int i = 0; i < numRequests; i++) { + resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello" + i)).get(); + assertSearchResponse(resp); + IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.ON_HEAP, false); + IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.DISK, false); + System.out.println("request number " + i); + } + + System.out.println("Num requests = " + numRequests); + + // 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(); + IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 1, TierType.ON_HEAP, false); + IndicesRequestCacheIT.assertCacheState(client, "index", 1, numRequests, TierType.DISK, false); + } +} diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 246bb7c57c07c..793d997b93522 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -34,7 +34,6 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; -import org.opensearch.action.IndicesRequestIT; import org.opensearch.action.admin.indices.alias.Alias; import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; import org.opensearch.action.search.SearchResponse; @@ -639,30 +638,13 @@ public void testProfileDisableCache() throws Exception { public void testCacheWithInvalidation() throws Exception { Client client = client(); - //int heapSizeBytes = 2000; // enough to fit 2 queries, as each is 687 B Settings.Builder builder = Settings.builder() .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) - .put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(2000)); - // Why is it appending "index." to the beginning of the key?? - - String heapSizeBytes = builder.get(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey()); - System.out.println("Current cap = " + heapSizeBytes); + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0); - client.admin().setSettings(Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(2000))); - - assertAcked( - client.admin() - .indices() - .prepareCreate("index") - .setMapping("k", "type=keyword") - .setSettings( - builder - ) - .get() - ); + assertAcked(client.admin().indices().prepareCreate("index").setMapping("k", "type=keyword").setSettings(builder).get()); indexRandom(true, client.prepareIndex("index").setSource("k", "hello")); ensureSearchable("index"); SearchResponse resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); @@ -676,8 +658,8 @@ public void testCacheWithInvalidation() throws Exception { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); assertSearchResponse(resp); // Should expect hit as here as refresh didn't happen - assertCacheState(client, "index", 1, 1, TierType.ON_HEAP); - assertCacheState(client, "index", 0, 1, TierType.DISK); + assertCacheState(client, "index", 1, 1, TierType.ON_HEAP, false); + assertCacheState(client, "index", 0, 1, TierType.DISK, false); assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Explicit refresh would invalidate cache @@ -686,13 +668,21 @@ public void testCacheWithInvalidation() throws Exception { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); assertSearchResponse(resp); // Should expect miss as key has changed due to change in IndexReader.CacheKey (due to refresh) - assertCacheState(client, "index", 1, 2, TierType.ON_HEAP); - assertCacheState(client, "index", 0, 2, TierType.DISK); - assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Shouldn't it just be the most recent query, since the first one was invalidated? (prob invalidation isnt in yet) + assertCacheState(client, "index", 1, 2, TierType.ON_HEAP, false); + assertCacheState(client, "index", 0, 2, TierType.DISK, false); + assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Shouldn't it just be the most recent query, since the first one was + // invalidated? (prob invalidation isnt in yet) // yeah - evictions = 0, its not in yet } - private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses, TierType tierType) { + protected static void assertCacheState( + Client client, + String index, + long expectedHits, + long expectedMisses, + TierType tierType, + boolean enforceZeroEvictions + ) { RequestCacheStats requestCacheStats = client.admin() .indices() .prepareStats(index) @@ -702,18 +692,32 @@ private static void assertCacheState(Client client, String index, long expectedH .getRequestCache(); // Check the hit count and miss count together so if they are not // correct we can see both values - System.out.println("mem size " + requestCacheStats.getMemorySize(tierType)); - assertEquals( - Arrays.asList(expectedHits, expectedMisses, 0L), - Arrays.asList(requestCacheStats.getHitCount(tierType), requestCacheStats.getMissCount(tierType), requestCacheStats.getEvictions(tierType)) - ); + ByteSizeValue memSize = requestCacheStats.getMemorySize(tierType); + if (memSize.getBytes() > 0) { + System.out.println("mem size " + memSize); + } + if (enforceZeroEvictions) { + assertEquals( + Arrays.asList(expectedHits, expectedMisses, 0L), + Arrays.asList( + requestCacheStats.getHitCount(tierType), + requestCacheStats.getMissCount(tierType), + requestCacheStats.getEvictions(tierType) + ) + ); + } else { + assertEquals( + Arrays.asList(expectedHits, expectedMisses), + Arrays.asList(requestCacheStats.getHitCount(tierType), requestCacheStats.getMissCount(tierType)) + ); + } } - private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { - assertCacheState(client, index, expectedHits, expectedMisses, TierType.ON_HEAP); + protected static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { + assertCacheState(client, index, expectedHits, expectedMisses, TierType.ON_HEAP, true); } - private static void assertNumCacheEntries(Client client, String index, long expectedEntries, TierType tierType) { + protected static void assertNumCacheEntries(Client client, String index, long expectedEntries, TierType tierType) { RequestCacheStats requestCacheStats = client.admin() .indices() .prepareStats(index) 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 42f7cfa15d3a6..3b87b529ee6aa 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 @@ -53,6 +53,7 @@ public class RequestCacheStats implements Writeable, ToXContentFragment { private Map map; + public RequestCacheStats() { this.map = new HashMap<>(); for (TierType tierType : TierType.values()) { @@ -64,22 +65,15 @@ public RequestCacheStats() { public RequestCacheStats(StreamInput in) throws IOException { this(); if (in.getVersion().onOrAfter(Version.V_3_0_0)) { - this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need to be registered? + this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need + // to be registered? } else { // objects from earlier versions only contain on-heap info, and do not have entries 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)); } } 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 23e3b141bdd37..7744b9c285a7d 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 @@ -58,10 +58,13 @@ public ShardRequestCache() { public RequestCacheStats stats() { // TODO: Change RequestCacheStats to support disk tier stats. // Changing this function to return a RequestCacheStats with stats from all tiers. - //return stats(TierType.ON_HEAP); + // return stats(TierType.ON_HEAP); return new RequestCacheStats(statsHolder); } + public void onHit(TierType tierType) { + statsHolder.get(tierType).hitCount.inc(); + } public void onMiss(TierType tierType) { statsHolder.get(tierType).missCount.inc(); 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 c7d7447a68459..92d057ab8fd9c 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 @@ -26,7 +26,6 @@ public class StatsHolder implements Serializable, Writeable, ToXContentFragment final CounterMetric missCount; final CounterMetric entries; - public StatsHolder() { this.totalMetric = new CounterMetric(); this.evictionsMetric = new CounterMetric(); @@ -49,7 +48,7 @@ public StatsHolder(long memorySize, long evictions, long hitCount, long missCoun this.entries.inc(entries); } - public StatsHolder(StreamInput in) throws IOException { + 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()); @@ -97,7 +96,11 @@ public long getEntries() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.humanReadableField(RequestCacheStats.Fields.MEMORY_SIZE_IN_BYTES, RequestCacheStats.Fields.MEMORY_SIZE, new ByteSizeValue(getMemorySize())); + builder.humanReadableField( + RequestCacheStats.Fields.MEMORY_SIZE_IN_BYTES, + RequestCacheStats.Fields.MEMORY_SIZE, + new ByteSizeValue(getMemorySize()) + ); builder.field(RequestCacheStats.Fields.EVICTIONS, getEvictions()); builder.field(RequestCacheStats.Fields.HIT_COUNT, getHitCount()); builder.field(RequestCacheStats.Fields.MISS_COUNT, getMissCount()); diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index bddd2ae3103c0..ff190852c67f6 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -115,7 +115,6 @@ public final class IndicesRequestCache implements TieredCacheEventListener tieredCacheService; private final IndicesService indicesService; - IndicesRequestCache(Settings settings, IndicesService indicesService) { this.size = INDICES_CACHE_QUERY_SIZE.get(settings); this.expire = INDICES_CACHE_QUERY_EXPIRE.exists(settings) ? INDICES_CACHE_QUERY_EXPIRE.get(settings) : null; 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 0b1adbe0f87c7..6992b8a441c0a 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 @@ -10,14 +10,10 @@ import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.core.common.bytes.BytesReference; -import org.opensearch.core.common.io.stream.BytesStream; import org.opensearch.core.common.io.stream.BytesStreamInput; -import org.opensearch.core.common.io.stream.StreamInput; -import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.indices.TierType; import org.opensearch.test.OpenSearchTestCase; -import java.io.IOException; import java.util.HashMap; import java.util.Map; @@ -28,9 +24,7 @@ public void testConstructorsAndAdd() throws Exception { assertTierState(emptyStats, tierType, 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)); RequestCacheStats heapOnlyStats = new RequestCacheStats(testHeapMap); for (TierType tierType : TierType.values()) { if (tierType == TierType.ON_HEAP) { @@ -41,12 +35,8 @@ public void testConstructorsAndAdd() throws Exception { } 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)); + testBothTiersMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10)); RequestCacheStats bothTiersStats = new RequestCacheStats(testBothTiersMap); assertTierState(bothTiersStats, TierType.ON_HEAP, 11, 12, 13, 14, 15); assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); @@ -57,16 +47,12 @@ public void testConstructorsAndAdd() throws Exception { } public void testSerialization() throws Exception { - // This test also implicitly tests StreamHolder serialization + // This test also implicitly tests StatsHolder serialization 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)); + testMap.put(TierType.DISK, new StatsHolder(6, 7, 8, 9, 10)); RequestCacheStats stats = new RequestCacheStats(testMap); stats.writeTo(os); BytesStreamInput is = new BytesStreamInput(BytesReference.toBytes(os.bytes())); @@ -91,5 +77,4 @@ private void assertTierState( assertEquals(missCount, stats.getMissCount(tierType)); assertEquals(entries, stats.getEntries(tierType)); } - } diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 6e5fcadf53a2e..c4af1b0f50952 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -149,7 +149,7 @@ public void testAddDirectToEhcache() throws Exception { String rKey = ((OpenSearchDirectoryReader) reader).getDelegatingCacheHelper().getDelegatingCacheKey().getId().toString(); IndicesRequestCache.Key key = cache.new Key(entity, termBytes, rKey); - BytesReference value = new BytesArray(new byte[]{0}); + BytesReference value = new BytesArray(new byte[] { 0 }); cache.tieredCacheHandler.getDiskCachingTier().put(key, value); BytesReference res = cache.tieredCacheHandler.getDiskCachingTier().get(key); From ad53d988b8ba2cb5b5b01c582a2ecd68a0dd287e Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 23 Oct 2023 15:25:45 -0700 Subject: [PATCH 07/18] cleaned up --- .../java/org/opensearch/indices/IndicesRequestCacheIT.java | 4 ---- .../opensearch/index/cache/request/RequestCacheStats.java | 5 +---- .../opensearch/index/cache/request/ShardRequestCache.java | 3 --- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 793d997b93522..ee66ab5142356 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -692,10 +692,6 @@ protected static void assertCacheState( .getRequestCache(); // Check the hit count and miss count together so if they are not // correct we can see both values - ByteSizeValue memSize = requestCacheStats.getMemorySize(tierType); - if (memSize.getBytes() > 0) { - System.out.println("mem size " + memSize); - } if (enforceZeroEvictions) { assertEquals( Arrays.asList(expectedHits, expectedMisses, 0L), 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 3b87b529ee6aa..c791495f3590c 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 @@ -65,8 +65,7 @@ public RequestCacheStats() { public RequestCacheStats(StreamInput in) throws IOException { this(); if (in.getVersion().onOrAfter(Version.V_3_0_0)) { - this.map = in.readMap(StreamInput::readString, StatsHolder::new); // does it know to use the right constructor? does it rly need - // to be registered? + this.map = in.readMap(StreamInput::readString, StatsHolder::new); } else { // objects from earlier versions only contain on-heap info, and do not have entries info long memorySize = in.readVLong(); @@ -85,8 +84,6 @@ public RequestCacheStats(Map inputMap) { } } - // can prob eliminate some of these constructors - public void add(RequestCacheStats stats) { for (String tier : stats.map.keySet()) { map.get(tier).add(stats.map.get(tier)); 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 7744b9c285a7d..7b9ef6c105f57 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 @@ -56,9 +56,6 @@ public ShardRequestCache() { } public RequestCacheStats stats() { - // TODO: Change RequestCacheStats to support disk tier stats. - // Changing this function to return a RequestCacheStats with stats from all tiers. - // return stats(TierType.ON_HEAP); return new RequestCacheStats(statsHolder); } From 79a56ac16ff429014c73d21025732292e904a14a Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 6 Nov 2023 09:44:27 -0800 Subject: [PATCH 08/18] Addressed comments besides moving IT tests into IndicesRequestCacheIT.java Signed-off-by: Peter Alfonsi --- .../IndicesRequestCacheDiskTierIT.java | 28 ++++++++++++++----- .../indices/IndicesRequestCacheIT.java | 20 ++++++++----- .../cache/request/RequestCacheStats.java | 18 ++++++------ 3 files changed, 42 insertions(+), 24 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java index 47bda50ca788a..e25fb0112ca01 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -37,6 +37,7 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Settings; import org.opensearch.core.common.unit.ByteSizeValue; +import org.opensearch.index.cache.request.RequestCacheStats; import org.opensearch.index.query.QueryBuilders; import org.opensearch.test.OpenSearchIntegTestCase; @@ -49,8 +50,7 @@ @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class IndicesRequestCacheDiskTierIT extends OpenSearchIntegTestCase { public void testDiskTierStats() throws Exception { - int heapSizeBytes = 1800; // enough to fit 2 queries, as each is 687 B - int requestSize = 687; // each request is 687 B + int heapSizeBytes = 4729; String node = internalCluster().startNode( Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), new ByteSizeValue(heapSizeBytes)) ); @@ -68,21 +68,35 @@ public void testDiskTierStats() throws Exception { ensureSearchable("index"); SearchResponse resp; + 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 + int numOnDisk = 5; int numRequests = heapSizeBytes / requestSize + numOnDisk; - for (int i = 0; i < numRequests; i++) { + for (int i = 1; i < numRequests; i++) { resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello" + i)).get(); assertSearchResponse(resp); IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.DISK, false); - System.out.println("request number " + i); } - - System.out.println("Num requests = " + numRequests); - // 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(); IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 1, numRequests, TierType.DISK, false); } + + private long getCacheSizeBytes(Client client, String index, TierType tierType) { + RequestCacheStats requestCacheStats = client.admin() + .indices() + .prepareStats(index) + .setRequestCache(true) + .get() + .getTotal() + .getRequestCache(); + return requestCacheStats.getMemorySizeInBytes(tierType); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index ee66ab5142356..63867de4b9217 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -638,13 +638,19 @@ public void testProfileDisableCache() throws Exception { public void testCacheWithInvalidation() throws Exception { Client client = client(); - - Settings.Builder builder = Settings.builder() - .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0); - - assertAcked(client.admin().indices().prepareCreate("index").setMapping("k", "type=keyword").setSettings(builder).get()); + assertAcked( + client.admin() + .indices() + .prepareCreate("index") + .setMapping("k", "type=keyword") + .setSettings( + Settings.builder() + .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + ) + .get() + ); indexRandom(true, client.prepareIndex("index").setSource("k", "hello")); ensureSearchable("index"); SearchResponse resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello")).get(); 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 c791495f3590c..d71ed6df48fec 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,18 +52,17 @@ */ public class RequestCacheStats implements Writeable, ToXContentFragment { - private Map map; - - public RequestCacheStats() { - this.map = new HashMap<>(); - for (TierType tierType : TierType.values()) { - map.put(tierType.getStringValue(), new StatsHolder()); + private Map map = new HashMap<>(){{ + 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() {} public RequestCacheStats(StreamInput in) throws IOException { - this(); if (in.getVersion().onOrAfter(Version.V_3_0_0)) { this.map = in.readMap(StreamInput::readString, StatsHolder::new); } else { @@ -78,7 +77,6 @@ public RequestCacheStats(StreamInput in) throws IOException { public RequestCacheStats(Map inputMap) { // Create a RequestCacheStats with multiple tiers' statistics - this(); for (TierType tierType : inputMap.keySet()) { map.put(tierType.getStringValue(), inputMap.get(tierType)); } From a96d62d755267829d7a5ae2a6555122fed99ae31 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 8 Nov 2023 15:11:55 -0800 Subject: [PATCH 09/18] first parts of cachevalue rework (nonfunctional) Signed-off-by: Peter Alfonsi --- .../common/cache/tier/CacheValue.java | 30 ++++++++++++++++ .../common/cache/tier/CachingTier.java | 2 +- .../cache/tier/DiskTierRequestStats.java | 35 +++++++++++++++++++ .../cache/tier/OnHeapTierRequestStats.java | 19 ++++++++++ .../common/cache/tier/TierRequestStats.java | 17 +++++++++ .../TieredCacheSpilloverStrategyService.java | 12 ------- 6 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java create mode 100644 server/src/main/java/org/opensearch/common/cache/tier/DiskTierRequestStats.java create mode 100644 server/src/main/java/org/opensearch/common/cache/tier/OnHeapTierRequestStats.java create mode 100644 server/src/main/java/org/opensearch/common/cache/tier/TierRequestStats.java diff --git a/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java b/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java new file mode 100644 index 0000000000000..a7a7039cfc770 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java @@ -0,0 +1,30 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.cache.tier; + +import org.opensearch.common.cache.tier.TierType; + +import java.util.Map; + +/** + * Represents a cache value, with its associated tier type where it is stored, + * and tier-specific stats for an individual request stored in a map. + * @param Type of value. + */ +public class CacheValue { + V value; + TierType source; + Map stats; + + CacheValue(V value, TierType source, Map stats) { + this.value = value; + this.source = source; + this.stats = stats; + } +} diff --git a/server/src/main/java/org/opensearch/common/cache/tier/CachingTier.java b/server/src/main/java/org/opensearch/common/cache/tier/CachingTier.java index 48fd5deadc111..2e94cacd0f40a 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/CachingTier.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/CachingTier.java @@ -18,7 +18,7 @@ */ public interface CachingTier { - V get(K key); + CacheValue get(K key); void put(K key, V value); diff --git a/server/src/main/java/org/opensearch/common/cache/tier/DiskTierRequestStats.java b/server/src/main/java/org/opensearch/common/cache/tier/DiskTierRequestStats.java new file mode 100644 index 0000000000000..33f36202c3f84 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/tier/DiskTierRequestStats.java @@ -0,0 +1,35 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.cache.tier; + +/** + * A class created by disk tier implementations containing disk-specific stats for a single request. + */ +public class DiskTierRequestStats implements TierRequestStats { + + private final long requestGetTimeNanos; + private final boolean requestReachedDisk; + + public DiskTierRequestStats(long requestGetTimeNanos, boolean requestReachedDisk) { + this.requestReachedDisk = requestReachedDisk; + this.requestGetTimeNanos = requestGetTimeNanos; + } + @Override + public TierType getTierType() { + return TierType.DISK; + } + + public long getRequestGetTimeNanos() { + return requestGetTimeNanos; + } + + public boolean getRequestReachedDisk() { + return requestReachedDisk; + } +} diff --git a/server/src/main/java/org/opensearch/common/cache/tier/OnHeapTierRequestStats.java b/server/src/main/java/org/opensearch/common/cache/tier/OnHeapTierRequestStats.java new file mode 100644 index 0000000000000..8f37f9e8a3141 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/tier/OnHeapTierRequestStats.java @@ -0,0 +1,19 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.cache.tier; + +/** + * A class created by disk tier implementations containing disk-specific stats for a single request. + */ +public class OnHeapTierRequestStats implements TierRequestStats { + @Override + public TierType getTierType() { + return TierType.ON_HEAP; + } +} diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TierRequestStats.java b/server/src/main/java/org/opensearch/common/cache/tier/TierRequestStats.java new file mode 100644 index 0000000000000..d156be7b3f028 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/tier/TierRequestStats.java @@ -0,0 +1,17 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.cache.tier; + +/** + * An interface for single-request tier-specific stats, which are produced on each request from a cache tier + * and then added to the correct shard's overall StatsHolder for the tier. + */ +public interface TierRequestStats { + TierType getTierType(); +} diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java index 153dbf9b330f5..0fcf0f03dc0ed 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java @@ -179,19 +179,7 @@ private Function> getValueFromTierCache(boolean trackStats) { }; } - /** - * Represents a cache value along with its associated tier type where it is stored. - * @param Type of value. - */ - public static class CacheValue { - V value; - TierType source; - CacheValue(V value, TierType source) { - this.value = value; - this.source = source; - } - } /** * Builder object From ff82eeadd8732c48e3e17cd9c6f2dc768dd28f5b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 8 Nov 2023 16:29:03 -0800 Subject: [PATCH 10/18] Almost finished statsholder impl side, found and deleted a lot of old tiered stuff in indices package --- .../common/cache/tier/CacheValue.java | 16 ++- .../cache/tier/EhCacheDiskCachingTier.java | 15 ++- .../cache/tier/OpenSearchOnHeapCache.java | 4 +- .../common/cache/tier/TierType.java | 1 + .../cache/tier/TieredCacheEventListener.java | 5 +- .../TieredCacheSpilloverStrategyService.java | 10 +- .../index/cache/request/DiskStatsHolder.java | 99 +++++++++++++++++++ .../cache/request/OnHeapStatsHolder.java | 37 +++++++ .../cache/request/RequestCacheStats.java | 1 + .../cache/request/ShardRequestCache.java | 9 +- .../index/cache/request/StatsHolder.java | 16 ++- .../AbstractIndexShardCacheEntity.java | 11 +-- .../indices/IndicesRequestCache.java | 13 +-- ...redCacheSpilloverStrategyServiceTests.java | 8 +- .../cache/request/RequestCacheStatsTests.java | 1 - .../indices/IndicesServiceCloseTests.java | 5 +- 16 files changed, 214 insertions(+), 37 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java create mode 100644 server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java diff --git a/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java b/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java index a7a7039cfc770..baeac79c172d9 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/CacheValue.java @@ -20,11 +20,23 @@ public class CacheValue { V value; TierType source; - Map stats; + TierRequestStats stats; - CacheValue(V value, TierType source, Map stats) { + CacheValue(V value, TierType source, TierRequestStats stats) { this.value = value; this.source = source; this.stats = stats; } + + public V getValue() { + return value; + } + + public TierType getSource() { + return source; + } + + public TierRequestStats getStats() { + return stats; + } } diff --git a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java index fad0c5b1f8552..57bfd2a753273 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java @@ -192,8 +192,19 @@ private CacheEventListenerConfigurationBuilder getListenerConfiguration(Builder< } @Override - public V get(K key) { - return valueSerializer.deserialize(cache.get(key)); + public CacheValue get(K key) { + // Optimize it by adding key store. + boolean reachedDisk = true; // TODO: Change this once we combine this with keystore integration + long now = System.nanoTime(); // Nanoseconds required; milliseconds might be too slow on an SSD + + V value = valueSerializer.deserialize(cache.get(key)); + + long tookTime = -1L; // This value will be ignored by stats accumulator if reachedDisk is false anyway + if (reachedDisk) { + tookTime = System.nanoTime() - now; + } + DiskTierRequestStats stats = new DiskTierRequestStats(tookTime, reachedDisk); + return new CacheValue<>(value, TierType.DISK, stats); } @Override diff --git a/server/src/main/java/org/opensearch/common/cache/tier/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/tier/OpenSearchOnHeapCache.java index 22d2f769507cf..c7cd065674de7 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/OpenSearchOnHeapCache.java @@ -66,8 +66,8 @@ public TierType getTierType() { } @Override - public V get(K key) { - return cache.get(key); + public CacheValue get(K key) { + return new CacheValue(cache.get(key), TierType.ON_HEAP, new OnHeapTierRequestStats()); } @Override diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TierType.java b/server/src/main/java/org/opensearch/common/cache/tier/TierType.java index 3ef4338030899..e3c18f6a1b5a3 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/TierType.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/TierType.java @@ -27,3 +27,4 @@ public String getStringValue() { return this.stringValue; } } + diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheEventListener.java b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheEventListener.java index 05b59bf16b282..d1c333e21818e 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheEventListener.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheEventListener.java @@ -17,11 +17,12 @@ */ public interface TieredCacheEventListener { - void onMiss(K key, TierType tierType); + void onMiss(K key, CacheValue cacheValue); void onRemoval(RemovalNotification notification); - void onHit(K key, V value, TierType tierType); + void onHit(K key, CacheValue cacheValue); void onCached(K key, V value, TierType tierType); + // Since only get() produces a CacheValue with stats, no need to modify onCached or onRemoval to have the CacheValue. } diff --git a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java index 0fcf0f03dc0ed..f8e037515ae6d 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyService.java @@ -164,15 +164,15 @@ private void setRemovalListeners() { private Function> getValueFromTierCache(boolean trackStats) { return key -> { for (CachingTier cachingTier : cachingTierList) { - V value = cachingTier.get(key); - if (value != null) { + CacheValue cacheValue = cachingTier.get(key); + if (cacheValue.value != null) { if (trackStats) { - tieredCacheEventListener.onHit(key, value, cachingTier.getTierType()); + tieredCacheEventListener.onHit(key, cacheValue); } - return new CacheValue<>(value, cachingTier.getTierType()); + return cacheValue; //new CacheValue<>(value, cachingTier.getTierType()); } if (trackStats) { - tieredCacheEventListener.onMiss(key, cachingTier.getTierType()); + tieredCacheEventListener.onMiss(key, cacheValue); } } return null; diff --git a/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java b/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java new file mode 100644 index 0000000000000..094b358333908 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java @@ -0,0 +1,99 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.cache.request; + +import org.opensearch.common.cache.tier.DiskTierRequestStats; +import org.opensearch.common.cache.tier.TierRequestStats; +import org.opensearch.common.cache.tier.TierType; +import org.opensearch.common.metrics.CounterMetric; +import org.opensearch.core.common.io.stream.StreamInput; +import org.opensearch.core.common.io.stream.StreamOutput; +import org.opensearch.core.xcontent.XContentBuilder; + +import java.io.IOException; + +public class DiskStatsHolder extends StatsHolder { + final CounterMetric totalGetTime; + final CounterMetric totalDiskReaches; // Number of times a get() has actually reached the disk + public DiskStatsHolder() { + super(); + this.totalGetTime = new CounterMetric(); + this.totalDiskReaches = new CounterMetric(); + } + + public DiskStatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries, long totalGetTime, long totalDiskReaches) { + super(memorySize, evictions, hitCount, missCount, entries); + this.totalGetTime = new CounterMetric(); + this.totalGetTime.inc(totalGetTime); + this.totalDiskReaches = new CounterMetric(); + this.totalDiskReaches.inc(totalDiskReaches); + } + + public DiskStatsHolder(StreamInput in) throws IOException { + this( + in.readVLong(), + in.readVLong(), + in.readVLong(), + in.readVLong(), + in.readVLong(), + in.readVLong(), + in.readVLong() + ); + } + + public long getTotalGetTime() { + return totalGetTime.count(); + } + + public long getTotalDiskReaches() { + return totalDiskReaches.count(); + } + + @Override + public void addRequestStats(TierRequestStats stats) { + assert stats.getTierType() == TierType.DISK; + DiskTierRequestStats diskStats = (DiskTierRequestStats) stats; + // TODO: Should this instead be accomplished with generics somehow + if (diskStats.getRequestReachedDisk()) { + this.totalDiskReaches.inc(); + this.totalGetTime.inc(diskStats.getRequestGetTimeNanos()); + } + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeVLong(totalGetTime.count()); + out.writeVLong(totalDiskReaches.count()); + } + + @Override + public void add(StatsHolder stats) { + assert stats.getClass() == DiskStatsHolder.class; + super.add(stats); + DiskStatsHolder diskStats = (DiskStatsHolder) stats; + totalDiskReaches.inc(diskStats.totalDiskReaches.count()); + totalGetTime.inc(diskStats.totalGetTime.count()); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + super.toXContent(builder, params); + builder.field(Fields.TOTAL_GET_TIME, getTotalGetTime()); + builder.field(Fields.TOTAL_DISK_REACHES, getTotalDiskReaches()); + return builder; + } + + static final class Fields { + static final String TOTAL_GET_TIME = "total_get_time_nanos"; + static final String TOTAL_DISK_REACHES = "total_disk_reaches"; + } + + +} diff --git a/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java b/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java new file mode 100644 index 0000000000000..5ef11f7850c5d --- /dev/null +++ b/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java @@ -0,0 +1,37 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.cache.request; + +import org.opensearch.common.cache.tier.TierRequestStats; +import org.opensearch.core.common.io.stream.StreamInput; + +import java.io.IOException; + + +/** + * This class accumulates stats for a single shard, for the on-heap tier. + * For now, on-heap tier has no unique stats, but future stats would be added here. + */ +public class OnHeapStatsHolder extends StatsHolder { + public OnHeapStatsHolder() { + super(); + } + + public OnHeapStatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries) { + super(memorySize, evictions, hitCount, missCount, entries); + } + + public OnHeapStatsHolder(StreamInput in) throws IOException { + super(in); + } + + @Override + public void addRequestStats(TierRequestStats stats) {} + +} 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 d71ed6df48fec..28c78c544fa0f 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 @@ -33,6 +33,7 @@ package org.opensearch.index.cache.request; import org.opensearch.Version; +import org.opensearch.common.cache.tier.TierType; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; 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 7b9ef6c105f57..7ef4fe46c122d 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 @@ -33,6 +33,7 @@ package org.opensearch.index.cache.request; import org.apache.lucene.util.Accountable; +import org.opensearch.common.cache.tier.CacheValue; import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.core.common.bytes.BytesReference; @@ -59,12 +60,12 @@ public RequestCacheStats stats() { return new RequestCacheStats(statsHolder); } - public void onHit(TierType tierType) { - statsHolder.get(tierType).hitCount.inc(); + public void onHit(CacheValue cacheValue) { + statsHolder.get(cacheValue.getSource()).hitCount.inc(); } - public void onMiss(TierType tierType) { - statsHolder.get(tierType).missCount.inc(); + public void onMiss(CacheValue cacheValue) { + statsHolder.get(cacheValue.getSource()).missCount.inc(); } 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..7596da018e842 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 @@ -8,6 +8,8 @@ package org.opensearch.index.cache.request; +import org.opensearch.common.cache.tier.TierRequestStats; +import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; @@ -19,13 +21,19 @@ import java.io.IOException; import java.io.Serializable; -public class StatsHolder implements Serializable, Writeable, ToXContentFragment { +/** + * The basic StatsHolder class, which accumulates shard-level stats that are common across all tiers. + * Used in ShardRequestCache. Extending classes also handle tier-specific stats for each tier. + */ +public abstract class StatsHolder implements Serializable, Writeable, ToXContentFragment { final CounterMetric totalMetric; final CounterMetric evictionsMetric; final CounterMetric hitCount; final CounterMetric missCount; final CounterMetric entries; + public static final Map + public StatsHolder() { this.totalMetric = new CounterMetric(); this.evictionsMetric = new CounterMetric(); @@ -107,4 +115,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(RequestCacheStats.Fields.ENTRIES, getEntries()); return builder; } + + /** + * Used to add stats for a single request (get call) to existing stats + * @param stats Stats for a single request + */ + public abstract void addRequestStats(TierRequestStats stats); } diff --git a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java index 81d4f545e0fd9..5d4dce28db1ca 100644 --- a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java +++ b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java @@ -34,6 +34,7 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; +import org.opensearch.common.cache.tier.CacheValue; import org.opensearch.common.cache.tier.TierType; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.index.cache.request.ShardRequestCache; @@ -58,15 +59,13 @@ public final void onCached(IndicesRequestCache.Key key, BytesReference value, Ti } @Override - public final void onHit(TierType tierType) { - // TODO: Handle tierType in stats - stats().onHit(); + final void onHit(CacheValue cacheValue) { + stats().onHit(cacheValue); } @Override - public final void onMiss(TierType tierType) { - // TODO: Handle tierType in stats - stats().onMiss(); + public final void onMiss(CacheValue cacheValue) { + stats().onMiss(cacheValue); } @Override diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index ff190852c67f6..6d2cbd0d1b1ba 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -40,6 +40,7 @@ import org.apache.lucene.util.RamUsageEstimator; import org.opensearch.common.CheckedSupplier; import org.opensearch.common.cache.RemovalNotification; +import org.opensearch.common.cache.tier.CacheValue; import org.opensearch.common.cache.tier.OnHeapCachingTier; import org.opensearch.common.cache.tier.OpenSearchOnHeapCache; import org.opensearch.common.cache.tier.TierType; @@ -145,8 +146,8 @@ void clear(CacheEntity entity) { } @Override - public void onMiss(Key key, TierType tierType) { - key.entity.onMiss(tierType); + public void onMiss(Key key, CacheValue cacheValue) { + key.entity.onMiss(cacheValue); } @Override @@ -155,8 +156,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, CacheValue cacheValue) { + key.entity.onHit(cacheValue); } @Override @@ -265,12 +266,12 @@ interface CacheEntity extends Accountable, Writeable { /** * Called each time this entity has a cache hit. */ - void onHit(TierType tierType); + void onHit(CacheValue cacheValue); /** * Called each time this entity has a cache miss. */ - void onMiss(TierType tierType); + void onMiss(CacheValue cacheValue); /** * Called when this entity instance is removed diff --git a/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java b/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java index bb7a22cc26037..bf4b46440733d 100644 --- a/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java +++ b/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java @@ -340,8 +340,8 @@ class MockTieredCacheEventListener implements TieredCacheEventListener cacheValue) { + enumMap.get(cacheValue.getSource()).missCount.inc(); } @Override @@ -352,8 +352,8 @@ public void onRemoval(RemovalNotification notification) { } @Override - public void onHit(K key, V value, TierType tierType) { - enumMap.get(tierType).hitCount.inc(); + public void onHit(K key, CacheValue cacheValue) { + enumMap.get(cacheValue.getSource()).hitCount.inc(); } @Override 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..8b57b094e347f 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 @@ -11,7 +11,6 @@ import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.io.stream.BytesStreamInput; -import org.opensearch.indices.TierType; import org.opensearch.test.OpenSearchTestCase; import java.util.HashMap; diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java index 5dd4eb504ec2f..accd7a29efb43 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java @@ -37,6 +37,7 @@ import org.opensearch.cluster.ClusterName; import org.opensearch.cluster.routing.allocation.DiskThresholdSettings; import org.opensearch.common.cache.RemovalNotification; +import org.opensearch.common.cache.tier.CacheValue; import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.OpenSearchExecutors; @@ -342,10 +343,10 @@ public Object getCacheIdentity() { } @Override - public void onHit(TierType tierType) {} + public void onHit(CacheValue cacheValue) {} @Override - public void onMiss(TierType tierType) {} + public void onMiss(CacheValue cacheValue) {} @Override public void onRemoval(RemovalNotification notification) {} From 6b6db22d42bd67ddf8e37caf22b7f0979c0652c0 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 9 Nov 2023 16:15:30 -0800 Subject: [PATCH 11/18] Finished cachevalue rework + disk stats implementation Signed-off-by: Peter Alfonsi --- .../index/cache/request/DiskStatsHolder.java | 99 ------------ .../cache/request/OnHeapStatsHolder.java | 37 ----- .../cache/request/RequestCacheStats.java | 64 ++++++-- .../cache/request/ShardRequestCache.java | 153 ++++++++++++++++-- .../index/cache/request/StatsHolder.java | 12 +- .../indices/IndicesRequestCache.java | 25 ++- .../tier/EhCacheDiskCachingTierTests.java | 14 +- ...redCacheSpilloverStrategyServiceTests.java | 8 +- .../cache/request/RequestCacheStatsTests.java | 30 +++- .../cache/request/ShardRequestCacheTests.java | 40 +++++ .../indices/IndicesRequestCacheTests.java | 120 -------------- 11 files changed, 294 insertions(+), 308 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java delete mode 100644 server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java create mode 100644 server/src/test/java/org/opensearch/index/cache/request/ShardRequestCacheTests.java diff --git a/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java b/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java deleted file mode 100644 index 094b358333908..0000000000000 --- a/server/src/main/java/org/opensearch/index/cache/request/DiskStatsHolder.java +++ /dev/null @@ -1,99 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.cache.request; - -import org.opensearch.common.cache.tier.DiskTierRequestStats; -import org.opensearch.common.cache.tier.TierRequestStats; -import org.opensearch.common.cache.tier.TierType; -import org.opensearch.common.metrics.CounterMetric; -import org.opensearch.core.common.io.stream.StreamInput; -import org.opensearch.core.common.io.stream.StreamOutput; -import org.opensearch.core.xcontent.XContentBuilder; - -import java.io.IOException; - -public class DiskStatsHolder extends StatsHolder { - final CounterMetric totalGetTime; - final CounterMetric totalDiskReaches; // Number of times a get() has actually reached the disk - public DiskStatsHolder() { - super(); - this.totalGetTime = new CounterMetric(); - this.totalDiskReaches = new CounterMetric(); - } - - public DiskStatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries, long totalGetTime, long totalDiskReaches) { - super(memorySize, evictions, hitCount, missCount, entries); - this.totalGetTime = new CounterMetric(); - this.totalGetTime.inc(totalGetTime); - this.totalDiskReaches = new CounterMetric(); - this.totalDiskReaches.inc(totalDiskReaches); - } - - public DiskStatsHolder(StreamInput in) throws IOException { - this( - in.readVLong(), - in.readVLong(), - in.readVLong(), - in.readVLong(), - in.readVLong(), - in.readVLong(), - in.readVLong() - ); - } - - public long getTotalGetTime() { - return totalGetTime.count(); - } - - public long getTotalDiskReaches() { - return totalDiskReaches.count(); - } - - @Override - public void addRequestStats(TierRequestStats stats) { - assert stats.getTierType() == TierType.DISK; - DiskTierRequestStats diskStats = (DiskTierRequestStats) stats; - // TODO: Should this instead be accomplished with generics somehow - if (diskStats.getRequestReachedDisk()) { - this.totalDiskReaches.inc(); - this.totalGetTime.inc(diskStats.getRequestGetTimeNanos()); - } - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - super.writeTo(out); - out.writeVLong(totalGetTime.count()); - out.writeVLong(totalDiskReaches.count()); - } - - @Override - public void add(StatsHolder stats) { - assert stats.getClass() == DiskStatsHolder.class; - super.add(stats); - DiskStatsHolder diskStats = (DiskStatsHolder) stats; - totalDiskReaches.inc(diskStats.totalDiskReaches.count()); - totalGetTime.inc(diskStats.totalGetTime.count()); - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - super.toXContent(builder, params); - builder.field(Fields.TOTAL_GET_TIME, getTotalGetTime()); - builder.field(Fields.TOTAL_DISK_REACHES, getTotalDiskReaches()); - return builder; - } - - static final class Fields { - static final String TOTAL_GET_TIME = "total_get_time_nanos"; - static final String TOTAL_DISK_REACHES = "total_disk_reaches"; - } - - -} diff --git a/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java b/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java deleted file mode 100644 index 5ef11f7850c5d..0000000000000 --- a/server/src/main/java/org/opensearch/index/cache/request/OnHeapStatsHolder.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.cache.request; - -import org.opensearch.common.cache.tier.TierRequestStats; -import org.opensearch.core.common.io.stream.StreamInput; - -import java.io.IOException; - - -/** - * This class accumulates stats for a single shard, for the on-heap tier. - * For now, on-heap tier has no unique stats, but future stats would be added here. - */ -public class OnHeapStatsHolder extends StatsHolder { - public OnHeapStatsHolder() { - super(); - } - - public OnHeapStatsHolder(long memorySize, long evictions, long hitCount, long missCount, long entries) { - super(memorySize, evictions, hitCount, missCount, entries); - } - - public OnHeapStatsHolder(StreamInput in) throws IOException { - super(in); - } - - @Override - public void addRequestStats(TierRequestStats stats) {} - -} 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 28c78c544fa0f..8563482e84bfd 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 @@ -32,7 +32,9 @@ package org.opensearch.index.cache.request; +import org.opensearch.OpenSearchException; import org.opensearch.Version; +import org.opensearch.common.cache.tier.TierRequestStats; import org.opensearch.common.cache.tier.TierType; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; @@ -43,6 +45,7 @@ import org.opensearch.common.cache.tier.TierType; import java.io.IOException; +import java.util.EnumMap; import java.util.HashMap; import java.util.Map; @@ -53,44 +56,64 @@ */ public class RequestCacheStats implements Writeable, ToXContentFragment { - private Map map = new HashMap<>(){{ - for (TierType tierType : TierType.values()) - { + private Map defaultStatsMap = new HashMap<>(){{ + 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 }} }; + private Map tierSpecificStatsMap = new HashMap<>(){{ + put(TierType.ON_HEAP.getStringValue(), new ShardRequestCache.OnHeapStatsAccumulator()); + put(TierType.DISK.getStringValue(), new ShardRequestCache.DiskStatsAccumulator()); + //checkTierSpecificMap(); // might yell, idk if the map is done until the block ends + }}; + public RequestCacheStats() {} public RequestCacheStats(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_3_0_0)) { - this.map = in.readMap(StreamInput::readString, StatsHolder::new); + this.defaultStatsMap = in.readMap(StreamInput::readString, StatsHolder::new); + // Manually fill the tier-specific stats map + this.tierSpecificStatsMap = new HashMap<>(); + tierSpecificStatsMap.put(TierType.ON_HEAP.getStringValue(), new ShardRequestCache.OnHeapStatsAccumulator(in)); + tierSpecificStatsMap.put(TierType.DISK.getStringValue(), new ShardRequestCache.DiskStatsAccumulator(in)); } else { // objects from earlier versions only contain on-heap info, and do not have entries 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.defaultStatsMap.put(TierType.ON_HEAP.getStringValue(), new StatsHolder(memorySize, evictions, hitCount, missCount, 0)); } + checkTierSpecificMap(); } - public RequestCacheStats(Map inputMap) { + public RequestCacheStats(Map defaultMap, Map tierSpecificMap) { // Create a RequestCacheStats with multiple tiers' statistics - for (TierType tierType : inputMap.keySet()) { - map.put(tierType.getStringValue(), inputMap.get(tierType)); + // The input maps don't have to have all tiers statistics available + for (TierType tierType : defaultMap.keySet()) { + defaultStatsMap.put(tierType.getStringValue(), defaultMap.get(tierType)); + } + for (TierType tierType : tierSpecificMap.keySet()) { + tierSpecificStatsMap.put(tierType.getStringValue(), tierSpecificMap.get(tierType)); } + checkTierSpecificMap(); } public void add(RequestCacheStats stats) { - for (String tier : stats.map.keySet()) { - map.get(tier).add(stats.map.get(tier)); + for (TierType tierType : TierType.values()) { + defaultStatsMap.get(tierType.getStringValue()).add(stats.defaultStatsMap.get(tierType.getStringValue())); + tierSpecificStatsMap.get(tierType.getStringValue()).add(stats.tierSpecificStatsMap.get(tierType.getStringValue())); } } private StatsHolder getTierStats(TierType tierType) { - return map.get(tierType.getStringValue()); + return defaultStatsMap.get(tierType.getStringValue()); + } + + ShardRequestCache.TierStatsAccumulator getTierSpecificStats(TierType tierType) { // pkg-private for testing + return tierSpecificStatsMap.get(tierType.getStringValue()); } public long getMemorySizeInBytes(TierType tierType) { @@ -146,10 +169,13 @@ public long getEntries() { @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)); // ? + out.writeMap(this.defaultStatsMap, StreamOutput::writeString, (o, v) -> v.writeTo(o)); // ? + // Manually write tier-specific stats map + tierSpecificStatsMap.get(TierType.ON_HEAP.getStringValue()).writeTo(out); + tierSpecificStatsMap.get(TierType.DISK.getStringValue()).writeTo(out); } else { // Write only on-heap values, and don't write entries metric - StatsHolder heapStats = map.get(TierType.ON_HEAP.getStringValue()); + StatsHolder heapStats = defaultStatsMap.get(TierType.ON_HEAP.getStringValue()); out.writeVLong(heapStats.getMemorySize()); out.writeVLong(heapStats.getEvictions()); out.writeVLong(heapStats.getHitCount()); @@ -162,12 +188,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(Fields.REQUEST_CACHE_STATS); // write on-heap stats outside of tiers object getTierStats(TierType.ON_HEAP).toXContent(builder, params); + getTierSpecificStats(TierType.ON_HEAP).toXContent(builder, params); 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); + defaultStatsMap.get(tier).toXContent(builder, params); + tierSpecificStatsMap.get(tier).toXContent(builder, params); builder.endObject(); } } @@ -176,6 +204,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } + private void checkTierSpecificMap() { + for (TierType tierType : TierType.values()) { + if (tierSpecificStatsMap.get(tierType.getStringValue()) == null) { + throw new OpenSearchException("Missing TierStatsAccumulator for TierType " + tierType.getStringValue()); + } + } + } + /** * Fields used for parsing and toXContent * 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 7ef4fe46c122d..d1baf3489cde0 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 @@ -33,11 +33,21 @@ package org.opensearch.index.cache.request; import org.apache.lucene.util.Accountable; +import org.opensearch.OpenSearchException; import org.opensearch.common.cache.tier.CacheValue; +import org.opensearch.common.cache.tier.DiskTierRequestStats; +import org.opensearch.common.cache.tier.OnHeapTierRequestStats; +import org.opensearch.common.cache.tier.TierRequestStats; import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.common.io.stream.StreamInput; +import org.opensearch.core.common.io.stream.StreamOutput; +import org.opensearch.core.common.io.stream.Writeable; +import org.opensearch.core.xcontent.ToXContentFragment; +import org.opensearch.core.xcontent.XContentBuilder; +import java.io.IOException; import java.util.EnumMap; /** @@ -48,35 +58,47 @@ public final class ShardRequestCache { // Holds stats common to all tiers - private final EnumMap statsHolder = new EnumMap<>(TierType.class); + private final EnumMap defaultStatsHolder = new EnumMap<>(TierType.class); + + // Holds tier-specific stats + private final EnumMap tierSpecificStatsHolder = new EnumMap<>(TierType.class); public ShardRequestCache() { + tierSpecificStatsHolder.put(TierType.ON_HEAP, new OnHeapStatsAccumulator()); + tierSpecificStatsHolder.put(TierType.DISK, new DiskStatsAccumulator()); for (TierType tierType : TierType.values()) { - statsHolder.put(tierType, new StatsHolder()); + defaultStatsHolder.put(tierType, new StatsHolder()); + if (tierSpecificStatsHolder.get(tierType) == null) { + throw new OpenSearchException("Missing TierStatsAccumulator for TierType " + tierType.getStringValue()); + } } } public RequestCacheStats stats() { - return new RequestCacheStats(statsHolder); + return new RequestCacheStats(defaultStatsHolder, tierSpecificStatsHolder); } public void onHit(CacheValue cacheValue) { - statsHolder.get(cacheValue.getSource()).hitCount.inc(); + TierType source = cacheValue.getSource(); + defaultStatsHolder.get(source).hitCount.inc(); + tierSpecificStatsHolder.get(source).addRequestStats(cacheValue.getStats()); } public void onMiss(CacheValue cacheValue) { - statsHolder.get(cacheValue.getSource()).missCount.inc(); + TierType source = cacheValue.getSource(); + defaultStatsHolder.get(cacheValue.getSource()).missCount.inc(); + tierSpecificStatsHolder.get(source).addRequestStats(cacheValue.getStats()); } public void onCached(Accountable key, BytesReference value, TierType tierType) { - statsHolder.get(tierType).totalMetric.inc(key.ramBytesUsed() + value.ramBytesUsed()); - statsHolder.get(tierType).entries.inc(); + defaultStatsHolder.get(tierType).totalMetric.inc(key.ramBytesUsed() + value.ramBytesUsed()); + defaultStatsHolder.get(tierType).entries.inc(); } public void onRemoval(Accountable key, BytesReference value, boolean evicted, TierType tierType) { if (evicted) { - statsHolder.get(tierType).evictionsMetric.inc(); + defaultStatsHolder.get(tierType).evictionsMetric.inc(); } long dec = 0; if (key != null) { @@ -85,7 +107,118 @@ public void onRemoval(Accountable key, BytesReference value, boolean evicted, Ti if (value != null) { dec += value.ramBytesUsed(); } - statsHolder.get(tierType).totalMetric.dec(dec); - statsHolder.get(tierType).entries.dec(); + defaultStatsHolder.get(tierType).totalMetric.dec(dec); + defaultStatsHolder.get(tierType).entries.dec(); + } + + /** + * An abstract whose extending classes accumulate tier-specific stats. + * All extending classes should provide a constructor like TierStatsAccumulator(StreamInput in) + * as well as a no-argument constructor + * @param The tier-specific implementation of TierRequestStats to use + */ + static abstract class TierStatsAccumulator implements Writeable, ToXContentFragment { + /** + * Add new stats from a single request to this holder. + * @param stats + */ + public abstract void addRequestStats(S stats); + + /** + * Add the stats from another TierStatsHolder to this TierStatsHolder. + * Used when combining stats across multiple shards. + * @param other The other TierStatsHolder. + */ + public abstract void add(TierStatsAccumulator other); + } + + /** + * This class accumulates on-heap-tier-specific stats for a single shard. + * For now, on-heap tier has no unique stats, but future stats would be added here. + */ + static class OnHeapStatsAccumulator extends TierStatsAccumulator { + OnHeapStatsAccumulator() {} + OnHeapStatsAccumulator(StreamInput in) {} + @Override + public void addRequestStats(OnHeapTierRequestStats stats) {} + + @Override + public void add(TierStatsAccumulator other) {} + + @Override + public void writeTo(StreamOutput out) throws IOException {} + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return builder; + } + } + + /** + * This class accumulates disk-tier-specific stats for a single shard. + */ + static class DiskStatsAccumulator extends TierStatsAccumulator { + final CounterMetric totalGetTime; + final CounterMetric totalDiskReaches; // Number of times a get() has actually reached the disk + public DiskStatsAccumulator() { + this.totalGetTime = new CounterMetric(); + this.totalDiskReaches = new CounterMetric(); + } + + public DiskStatsAccumulator(long totalGetTime, long totalDiskReaches) { + this.totalGetTime = new CounterMetric(); + this.totalGetTime.inc(totalGetTime); + this.totalDiskReaches = new CounterMetric(); + this.totalDiskReaches.inc(totalDiskReaches); + } + + public DiskStatsAccumulator(StreamInput in) throws IOException { + this( + in.readVLong(), + in.readVLong() + ); + } + + public long getTotalGetTime() { + return totalGetTime.count(); + } + + public long getTotalDiskReaches() { + return totalDiskReaches.count(); + } + + @Override + public void addRequestStats(DiskTierRequestStats stats) { + if (stats.getRequestReachedDisk()) { + this.totalDiskReaches.inc(); + this.totalGetTime.inc(stats.getRequestGetTimeNanos()); + } + } + + @Override + public void add(TierStatsAccumulator other) { + assert other.getClass() == DiskStatsAccumulator.class; + DiskStatsAccumulator castOther = (DiskStatsAccumulator) other; + this.totalDiskReaches.inc(castOther.totalDiskReaches.count()); + this.totalGetTime.inc(castOther.totalGetTime.count()); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVLong(totalGetTime.count()); + out.writeVLong(totalDiskReaches.count()); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.field(Fields.TOTAL_GET_TIME, getTotalGetTime()); + builder.field(Fields.TOTAL_DISK_REACHES, getTotalDiskReaches()); + return builder; + } + + static final class Fields { // Used for field names in API response + static final String TOTAL_GET_TIME = "total_get_time_nanos"; + static final String TOTAL_DISK_REACHES = "total_disk_reaches"; + } } } 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 7596da018e842..300689cf09dff 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 @@ -20,20 +20,20 @@ import java.io.IOException; import java.io.Serializable; +import java.util.HashMap; +import java.util.Map; /** * The basic StatsHolder class, which accumulates shard-level stats that are common across all tiers. * Used in ShardRequestCache. Extending classes also handle tier-specific stats for each tier. */ -public abstract class StatsHolder implements Serializable, Writeable, ToXContentFragment { +public class StatsHolder implements Serializable, Writeable, ToXContentFragment { final CounterMetric totalMetric; final CounterMetric evictionsMetric; final CounterMetric hitCount; final CounterMetric missCount; final CounterMetric entries; - public static final Map - public StatsHolder() { this.totalMetric = new CounterMetric(); this.evictionsMetric = new CounterMetric(); @@ -115,10 +115,4 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(RequestCacheStats.Fields.ENTRIES, getEntries()); return builder; } - - /** - * Used to add stats for a single request (get call) to existing stats - * @param stats Stats for a single request - */ - public abstract void addRequestStats(TierRequestStats stats); } diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 6d2cbd0d1b1ba..6195bf017df29 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -41,6 +41,7 @@ import org.opensearch.common.CheckedSupplier; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.tier.CacheValue; +import org.opensearch.common.cache.tier.EhCacheDiskCachingTier; import org.opensearch.common.cache.tier.OnHeapCachingTier; import org.opensearch.common.cache.tier.OpenSearchOnHeapCache; import org.opensearch.common.cache.tier.TierType; @@ -127,12 +128,28 @@ public final class IndicesRequestCache implements TieredCacheEventListener k.ramBytesUsed() + v.ramBytesUsed() ).setMaximumWeight(sizeInBytes).setExpireAfterAccess(expire).build(); + // enabling this for testing purposes. Remove/tweak!! + //long CACHE_SIZE_IN_BYTES = 1000000L; + //String SETTING_PREFIX = "indices.request.cache"; + + /*EhCacheDiskCachingTier ehcacheDiskTier = new EhCacheDiskCachingTier.Builder() + .setKeyType(Key.class) + .setValueType(BytesReference.class) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setSettings(settings) + .setThreadPoolAlias("ehcacheTest") + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setStoragePath("/tmp") + .setSettingPrefix(SETTING_PREFIX) + .build();*/ + // Initialize tiered cache service. TODO: Enable Disk tier when tiered support is turned on. - tieredCacheService = new TieredCacheSpilloverStrategyService.Builder() - .setOnHeapCachingTier(openSearchOnHeapCache) - .setTieredCacheEventListener(this) - .build(); + tieredCacheService = new TieredCacheSpilloverStrategyService.Builder().setOnHeapCachingTier( + openSearchOnHeapCache + ) + //.setOnDiskCachingTier(ehcacheDiskTier) + .setTieredCacheEventListener(this).build(); } @Override diff --git a/server/src/test/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTierTests.java b/server/src/test/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTierTests.java index f443af615d8ec..afa03cc9aab24 100644 --- a/server/src/test/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTierTests.java +++ b/server/src/test/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTierTests.java @@ -58,7 +58,11 @@ public void testBasicGetAndPut() throws IOException { ehCacheDiskCachingTierNew.put(entry.getKey(), entry.getValue()); } for (Map.Entry entry : keyValueMap.entrySet()) { - String value = ehCacheDiskCachingTierNew.get(entry.getKey()); + CacheValue value = ehCacheDiskCachingTierNew.get(entry.getKey()); + assertEquals(entry.getValue(), value.value); + assertEquals(TierType.DISK, value.getSource()); + assertTrue(((DiskTierRequestStats) value.getStats()).getRequestReachedDisk()); + assertTrue(((DiskTierRequestStats) value.getStats()).getRequestGetTimeNanos() > 0); } ehCacheDiskCachingTierNew.close(); } @@ -92,7 +96,7 @@ public void testBasicGetAndPutBytesReference() throws Exception { ehCacheDiskCachingTier.put(entry.getKey(), entry.getValue()); } for (Map.Entry entry : keyValueMap.entrySet()) { - BytesReference value = ehCacheDiskCachingTier.get(entry.getKey()); + BytesReference value = ehCacheDiskCachingTier.get(entry.getKey()).value; assertEquals(entry.getValue(), value); } ehCacheDiskCachingTier.close(); @@ -135,8 +139,8 @@ public void testConcurrentPut() throws Exception { phaser.arriveAndAwaitAdvance(); // Will trigger parallel puts above. countDownLatch.await(); // Wait for all threads to finish for (Map.Entry entry : keyValueMap.entrySet()) { - String value = ehCacheDiskCachingTierNew.get(entry.getKey()); - assertEquals(entry.getValue(), value); + CacheValue value = ehCacheDiskCachingTierNew.get(entry.getKey()); + assertEquals(entry.getValue(), value.value); } ehCacheDiskCachingTierNew.close(); } @@ -175,7 +179,7 @@ public void testEhcacheParallelGets() throws Exception { for (Map.Entry entry : keyValueMap.entrySet()) { threads[j] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); - assertEquals(entry.getValue(), ehCacheDiskCachingTierNew.get(entry.getKey())); + assertEquals(entry.getValue(), ehCacheDiskCachingTierNew.get(entry.getKey()).value); countDownLatch.countDown(); }); threads[j].start(); diff --git a/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java b/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java index bf4b46440733d..a85d82118ff66 100644 --- a/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java +++ b/server/src/test/java/org/opensearch/common/cache/tier/TieredCacheSpilloverStrategyServiceTests.java @@ -254,8 +254,8 @@ class MockOnHeapCacheTier implements OnHeapCachingTier, RemovalListe } @Override - public V get(K key) { - return this.onHeapCacheTier.get(key); + public CacheValue get(K key) { + return new CacheValue(this.onHeapCacheTier.get(key), TierType.ON_HEAP, new OnHeapTierRequestStats()); } @Override @@ -381,8 +381,8 @@ class MockDiskCachingTier implements DiskCachingTier, RemovalListene } @Override - public V get(K key) { - return this.diskTier.get(key); + public CacheValue get(K key) { + return new CacheValue<>(this.diskTier.get(key), TierType.DISK, new DiskTierRequestStats(0L, true)); } @Override 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 8b57b094e347f..b481cf762f9ec 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 @@ -8,6 +8,7 @@ package org.opensearch.index.cache.request; +import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.io.stream.BytesStreamInput; @@ -22,27 +23,35 @@ public void testConstructorsAndAdd() throws Exception { for (TierType tierType : TierType.values()) { assertTierState(emptyStats, tierType, 0, 0, 0, 0, 0); } + assertDiskStatsState(emptyStats, 0, 0); Map testHeapMap = new HashMap<>(); testHeapMap.put(TierType.ON_HEAP, new StatsHolder(1, 2, 3, 4, 5)); - RequestCacheStats heapOnlyStats = new RequestCacheStats(testHeapMap); + Map tierSpecificMap = new HashMap<>(); + tierSpecificMap.put(TierType.DISK, new ShardRequestCache.DiskStatsAccumulator(6, 7)); + RequestCacheStats heapAndSpecificOnlyStats = new RequestCacheStats(testHeapMap, tierSpecificMap); for (TierType tierType : TierType.values()) { if (tierType == TierType.ON_HEAP) { - assertTierState(heapOnlyStats, tierType, 1, 2, 3, 4, 5); + assertTierState(heapAndSpecificOnlyStats, tierType, 1, 2, 3, 4, 5); } else { - assertTierState(heapOnlyStats, tierType, 0, 0, 0, 0, 0); + assertTierState(heapAndSpecificOnlyStats, tierType, 0, 0, 0, 0, 0); } } + assertDiskStatsState(heapAndSpecificOnlyStats, 6, 7); 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)); - RequestCacheStats bothTiersStats = new RequestCacheStats(testBothTiersMap); + Map newTierSpecificMap = new HashMap<>(); + newTierSpecificMap.put(TierType.ON_HEAP, new ShardRequestCache.OnHeapStatsAccumulator()); + newTierSpecificMap.put(TierType.DISK, new ShardRequestCache.DiskStatsAccumulator(8, 9)); + RequestCacheStats bothTiersStats = new RequestCacheStats(testBothTiersMap, newTierSpecificMap); assertTierState(bothTiersStats, TierType.ON_HEAP, 11, 12, 13, 14, 15); assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); - bothTiersStats.add(heapOnlyStats); + bothTiersStats.add(heapAndSpecificOnlyStats); assertTierState(bothTiersStats, TierType.ON_HEAP, 12, 14, 16, 18, 20); assertTierState(bothTiersStats, TierType.DISK, 6, 7, 8, 9, 10); + assertDiskStatsState(bothTiersStats, 14, 16); } public void testSerialization() throws Exception { @@ -52,13 +61,17 @@ public void testSerialization() throws Exception { 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)); - RequestCacheStats stats = new RequestCacheStats(testMap); + Map tierSpecificMap = new HashMap<>(); + tierSpecificMap.put(TierType.ON_HEAP, new ShardRequestCache.OnHeapStatsAccumulator()); + tierSpecificMap.put(TierType.DISK, new ShardRequestCache.DiskStatsAccumulator(20, 21)); + RequestCacheStats stats = new RequestCacheStats(testMap, tierSpecificMap); 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); + assertDiskStatsState(deserialized, 20, 21); } private void assertTierState( @@ -76,4 +89,9 @@ private void assertTierState( assertEquals(missCount, stats.getMissCount(tierType)); assertEquals(entries, stats.getEntries(tierType)); } + + private void assertDiskStatsState(RequestCacheStats stats, long totalGetTime, long totalDiskReaches) { + assertEquals(totalGetTime, ((ShardRequestCache.DiskStatsAccumulator) stats.getTierSpecificStats(TierType.DISK)).getTotalGetTime()); + assertEquals(totalDiskReaches, ((ShardRequestCache.DiskStatsAccumulator) stats.getTierSpecificStats(TierType.DISK)).getTotalDiskReaches()); + } } diff --git a/server/src/test/java/org/opensearch/index/cache/request/ShardRequestCacheTests.java b/server/src/test/java/org/opensearch/index/cache/request/ShardRequestCacheTests.java new file mode 100644 index 0000000000000..8b97924343152 --- /dev/null +++ b/server/src/test/java/org/opensearch/index/cache/request/ShardRequestCacheTests.java @@ -0,0 +1,40 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.cache.request; + + +import org.opensearch.common.cache.tier.DiskTierRequestStats; +import org.opensearch.test.OpenSearchTestCase; + +public class ShardRequestCacheTests extends OpenSearchTestCase { + // Serialization and getter logic is implicitly tested in RequestCacheStatsTests.java, + // in this file, check logic for StatsHolder.TierStatsAccumulator implementations + + public void testInit() throws Exception { + ShardRequestCache src = new ShardRequestCache(); + RequestCacheStats rcs = src.stats(); + } + public void testDiskStatsAccumulator() throws Exception { + ShardRequestCache.DiskStatsAccumulator acc = new ShardRequestCache.DiskStatsAccumulator(); + DiskTierRequestStats reachedDiskReqStats = new DiskTierRequestStats(145L, true); + acc.addRequestStats(reachedDiskReqStats); + assertEquals(1, acc.getTotalDiskReaches()); + assertEquals(145, acc.getTotalGetTime()); + DiskTierRequestStats noDiskReqStats = new DiskTierRequestStats(391392L, false); + acc.addRequestStats(noDiskReqStats); + assertEquals(1, acc.getTotalDiskReaches()); + assertEquals(145, acc.getTotalGetTime()); + + ShardRequestCache.DiskStatsAccumulator other = new ShardRequestCache.DiskStatsAccumulator(); + other.addRequestStats(new DiskTierRequestStats(1L, true)); + acc.add(other); + assertEquals(146, acc.getTotalGetTime()); + assertEquals(2, acc.getTotalDiskReaches()); + } +} diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index c4af1b0f50952..5fbffe6906d56 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -124,126 +124,6 @@ public void testBasicOperationsCache() throws Exception { IOUtils.close(reader, writer, dir, cache); assertEquals(0, cache.numRegisteredCloseListeners()); - cache.closeDiskTier(); - } - - public void testAddDirectToEhcache() throws Exception { - // this test is for debugging serialization issues and can eventually be removed - // Put breakpoints at line 260 of AbstractOffHeapStore to catch serialization errors - // that would otherwise fail silently - ShardRequestCache requestCacheStats = new ShardRequestCache(); - Settings.Builder settingsBuilder = Settings.builder(); - long heapSizeBytes = 1000; - settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); - IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); - - // set up a key - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - AtomicBoolean indexShard = new AtomicBoolean(true); - TestEntity entity = new TestEntity(requestCacheStats, indexShard); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - String rKey = ((OpenSearchDirectoryReader) reader).getDelegatingCacheHelper().getDelegatingCacheKey().getId().toString(); - IndicesRequestCache.Key key = cache.new Key(entity, termBytes, rKey); - - BytesReference value = new BytesArray(new byte[] { 0 }); - cache.tieredCacheHandler.getDiskCachingTier().put(key, value); - - BytesReference res = cache.tieredCacheHandler.getDiskCachingTier().get(key); - assertEquals(value, res); - assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); - - IOUtils.close(reader, writer, dir, cache); - cache.closeDiskTier(); - } - - public void testSpillover() throws Exception { - // fill the on-heap cache until we spill over - ShardRequestCache requestCacheStats = new ShardRequestCache(); - Settings.Builder settingsBuilder = Settings.builder(); - long heapSizeBytes = 1000; // each of these queries is 131 bytes, so we can fit 7 in the heap cache - int heapKeySize = 131; - int maxNumInHeap = 1000 / heapKeySize; - settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); - IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); - - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - AtomicBoolean indexShard = new AtomicBoolean(true); - - 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++) { - TermQueryBuilder termQuery = new TermQueryBuilder("id", String.valueOf(i)); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - String rKey = ((OpenSearchDirectoryReader) reader).getDelegatingCacheHelper().getDelegatingCacheKey().getId().toString(); - termBytesArr[i] = termBytes; - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); - } - // get value from disk cache, the first key should have been evicted - BytesReference firstValue = cache.getOrCompute(entity, loader, reader, termBytesArr[0]); - - assertEquals(maxNumInHeap * heapKeySize, requestCacheStats.stats().getMemorySizeInBytes()); - // TODO: disk weight bytes - assertEquals(1, requestCacheStats.stats().getEvictions()); - assertEquals(1, requestCacheStats.stats().getHitCount(TierType.DISK)); - assertEquals(maxNumInHeap + 1, requestCacheStats.stats().getMissCount(TierType.DISK)); - assertEquals(0, requestCacheStats.stats().getHitCount()); - assertEquals(maxNumInHeap + 2, requestCacheStats.stats().getMissCount()); - assertEquals(maxNumInHeap, cache.tieredCacheHandler.count(TierType.ON_HEAP)); - assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); - - // get a value from heap cache, second key should still be there - BytesReference secondValue = cache.getOrCompute(entity, loader, reader, termBytesArr[1]); - // get the value on disk cache again - BytesReference firstValueAgain = cache.getOrCompute(entity, loader, reader, termBytesArr[0]); - - assertEquals(1, requestCacheStats.stats().getEvictions()); - assertEquals(2, requestCacheStats.stats().getHitCount(TierType.DISK)); - assertEquals(maxNumInHeap + 1, requestCacheStats.stats().getMissCount(TierType.DISK)); - assertEquals(1, requestCacheStats.stats().getHitCount()); - assertEquals(maxNumInHeap + 3, requestCacheStats.stats().getMissCount()); - assertEquals(maxNumInHeap, cache.tieredCacheHandler.count(TierType.ON_HEAP)); - assertEquals(1, cache.tieredCacheHandler.count(TierType.DISK)); - - IOUtils.close(reader, writer, dir, cache); - cache.closeDiskTier(); - } - - public void testDiskGetTimeEWMA() throws Exception { - ShardRequestCache requestCacheStats = new ShardRequestCache(); - Settings.Builder settingsBuilder = Settings.builder(); - long heapSizeBytes = 0; // skip directly to disk cache - settingsBuilder.put("indices.requests.cache.size", new ByteSizeValue(heapSizeBytes)); - IndicesRequestCache cache = new IndicesRequestCache(settingsBuilder.build(), getInstanceFromNode(IndicesService.class)); - - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - AtomicBoolean indexShard = new AtomicBoolean(true); - - TestEntity entity = new TestEntity(requestCacheStats, indexShard); - Loader loader = new Loader(reader, 0); - - for (int i = 0; i < 50; i++) { - TermQueryBuilder termQuery = new TermQueryBuilder("id", String.valueOf(i)); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); - // on my machine get time EWMA converges to ~0.025 ms, but it does have an SSD - assertTrue(cache.tieredCacheHandler.diskGetTimeMillisEWMA() > 0); - } - - IOUtils.close(reader, writer, dir, cache); - cache.closeDiskTier(); } public void testCacheDifferentReaders() throws Exception { From 54c3f4909a5b93b86a4f52dd3de2c659e517b6b5 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 14 Nov 2023 14:48:35 -0800 Subject: [PATCH 12/18] Fixed issues with closing disk tier in tests, added IT for disk specific stats Signed-off-by: Peter Alfonsi --- .../IndicesRequestCacheDiskTierIT.java | 36 +++++++++++++++++++ .../cache/request/RequestCacheStats.java | 5 ++- .../cache/request/ShardRequestCache.java | 4 +-- .../indices/IndicesRequestCache.java | 3 ++ 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java index e25fb0112ca01..376c2fb143998 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -38,6 +38,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.cache.request.RequestCacheStats; +import org.opensearch.index.cache.request.ShardRequestCache; import org.opensearch.index.query.QueryBuilders; import org.opensearch.test.OpenSearchIntegTestCase; @@ -83,10 +84,28 @@ public void testDiskTierStats() throws Exception { IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.DISK, false); } + + // So far, disk-specific stats should be 0, as keystore has prevented any actual disk reaches + long tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 0, -1, 0); + // 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(); IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 1, numRequests, TierType.DISK, false); + tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 1, 0, -1); + + // We make another actual request that should be in the disk tier. Disk specific stats should keep incrementing + resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello1")).get(); + IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 2, TierType.ON_HEAP, false); + IndicesRequestCacheIT.assertCacheState(client, "index", 2, numRequests, TierType.DISK, false); + tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 2, tookTimeSoFar, -1); + + // A final request for something in neither tier shouldn't increment disk specific stats + resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello" + numRequests)).get(); + IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 3, TierType.ON_HEAP, false); + IndicesRequestCacheIT.assertCacheState(client, "index", 2, numRequests + 1, TierType.DISK, false); + assertDiskTierSpecificStats(client, "index", 2, tookTimeSoFar, tookTimeSoFar); + } private long getCacheSizeBytes(Client client, String index, TierType tierType) { @@ -99,4 +118,21 @@ private long getCacheSizeBytes(Client client, String index, TierType tierType) { .getRequestCache(); return requestCacheStats.getMemorySizeInBytes(tierType); } + + private long assertDiskTierSpecificStats(Client client, String index, long totalDiskReaches, long totalGetTimeLowerBound, long totalGetTimeUpperBound) { + // set bounds to -1 to ignore them + RequestCacheStats requestCacheStats = client.admin() + .indices() + .prepareStats(index) + .setRequestCache(true) + .get() + .getTotal() + .getRequestCache(); + ShardRequestCache.DiskStatsAccumulator specStats = requestCacheStats.getDiskSpecificStats(); + assertEquals(totalDiskReaches, specStats.getTotalDiskReaches()); + long tookTime = specStats.getTotalGetTime(); + assertTrue(tookTime >= totalGetTimeLowerBound || totalGetTimeLowerBound < 0); + assertTrue(tookTime <= totalGetTimeUpperBound || totalGetTimeUpperBound < 0); + return tookTime; // Return for use in next check + } } 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 8563482e84bfd..7de71015e621e 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 @@ -66,7 +66,6 @@ public class RequestCacheStats implements Writeable, ToXContentFragment { private Map tierSpecificStatsMap = new HashMap<>(){{ put(TierType.ON_HEAP.getStringValue(), new ShardRequestCache.OnHeapStatsAccumulator()); put(TierType.DISK.getStringValue(), new ShardRequestCache.DiskStatsAccumulator()); - //checkTierSpecificMap(); // might yell, idk if the map is done until the block ends }}; public RequestCacheStats() {} @@ -116,6 +115,10 @@ ShardRequestCache.TierStatsAccumulator getTierSpecificStats(TierType tierType) { return tierSpecificStatsMap.get(tierType.getStringValue()); } + public ShardRequestCache.DiskStatsAccumulator getDiskSpecificStats() { + return (ShardRequestCache.DiskStatsAccumulator) tierSpecificStatsMap.get(TierType.DISK.getStringValue()); + } + public long getMemorySizeInBytes(TierType tierType) { return getTierStats(tierType).totalMetric.count(); } 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 d1baf3489cde0..51bf8de2f7d15 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 @@ -136,7 +136,7 @@ static abstract class TierStatsAccumulator implement * This class accumulates on-heap-tier-specific stats for a single shard. * For now, on-heap tier has no unique stats, but future stats would be added here. */ - static class OnHeapStatsAccumulator extends TierStatsAccumulator { + public static class OnHeapStatsAccumulator extends TierStatsAccumulator { OnHeapStatsAccumulator() {} OnHeapStatsAccumulator(StreamInput in) {} @Override @@ -157,7 +157,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws /** * This class accumulates disk-tier-specific stats for a single shard. */ - static class DiskStatsAccumulator extends TierStatsAccumulator { + public static class DiskStatsAccumulator extends TierStatsAccumulator { final CounterMetric totalGetTime; final CounterMetric totalDiskReaches; // Number of times a get() has actually reached the disk public DiskStatsAccumulator() { diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 6195bf017df29..4c7911c89a465 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -155,6 +155,9 @@ public final class IndicesRequestCache implements TieredCacheEventListener Date: Tue, 14 Nov 2023 15:46:21 -0800 Subject: [PATCH 13/18] removed print statement Signed-off-by: Peter Alfonsi --- .../org/opensearch/indices/IndicesRequestCacheDiskTierIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java index 376c2fb143998..bef9bbd78b176 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -71,7 +71,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 From 108dd414c3222cff638e6955b732f6034bbb41eb Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 16 Nov 2023 13:21:59 -0800 Subject: [PATCH 14/18] Removed incorrect line from test Signed-off-by: Peter Alfonsi --- .../java/org/opensearch/indices/IndicesRequestCacheIT.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 63867de4b9217..bdec4283c368a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -676,9 +676,8 @@ public void testCacheWithInvalidation() throws Exception { // Should expect miss as key has changed due to change in IndexReader.CacheKey (due to refresh) assertCacheState(client, "index", 1, 2, TierType.ON_HEAP, false); assertCacheState(client, "index", 0, 2, TierType.DISK, false); - assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Shouldn't it just be the most recent query, since the first one was - // invalidated? (prob invalidation isnt in yet) - // yeah - evictions = 0, its not in yet + + //assertNumCacheEntries(client, "index", 1, TierType.ON_HEAP); // Evictions won't be 1 until the cache cleaner runs every minute } protected static void assertCacheState( From bed3be28211cb4e86cd716a930edacaf6d87b7df Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 30 Nov 2023 13:24:22 -0800 Subject: [PATCH 15/18] added broken javadoc --- .../org/opensearch/index/cache/request/ShardRequestCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 51bf8de2f7d15..96a04768e3805 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 @@ -120,7 +120,7 @@ public void onRemoval(Accountable key, BytesReference value, boolean evicted, Ti static abstract class TierStatsAccumulator implements Writeable, ToXContentFragment { /** * Add new stats from a single request to this holder. - * @param stats + * @param stats The stats from a single request to add */ public abstract void addRequestStats(S stats); From 14fbbc501705fd5270245dcb7dafe7e21f5a439c Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 2 Jan 2024 15:45:04 -0800 Subject: [PATCH 16/18] cleanup commit 1 Signed-off-by: Peter Alfonsi --- .../IndicesRequestCacheDiskTierIT.java | 34 +++++++++++++------ .../cache/tier/EhCacheDiskCachingTier.java | 1 + .../AbstractIndexShardCacheEntity.java | 12 ++++--- .../indices/IndicesRequestCache.java | 26 ++++++++------ 4 files changed, 47 insertions(+), 26 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java index bef9bbd78b176..5da0b545e215f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java @@ -35,6 +35,7 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.cache.tier.TierType; import org.opensearch.common.settings.Settings; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.cache.request.RequestCacheStats; @@ -84,26 +85,33 @@ public void testDiskTierStats() throws Exception { IndicesRequestCacheIT.assertCacheState(client, "index", 0, i + 1, TierType.DISK, false); } + + long diskReachesSoFar = getDiskStatsAccumulator(client, "index").getTotalDiskReaches(); + long tookTimeSoFar = getDiskStatsAccumulator(client, "index").getTotalGetTime(); // So far, disk-specific stats should be 0, as keystore has prevented any actual disk reaches - long tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 0, -1, 0); + // assertEquals(diskReachesSoFar, 0); // TODO: Once keystore is integrated, this value should be 0 + // assertEquals(getTimeSoFar, 0); // TODO: Once keystore is integrated, this value should be 0 + + // long tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 0, -1, 0); // TODO: Uncomment once keystore is integrated // 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(); IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 1, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 1, numRequests, TierType.DISK, false); - tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 1, 0, -1); + tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 1 + diskReachesSoFar, tookTimeSoFar, -1); // We make another actual request that should be in the disk tier. Disk specific stats should keep incrementing resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello1")).get(); IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 2, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 2, numRequests, TierType.DISK, false); - tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 2, tookTimeSoFar, -1); + tookTimeSoFar = assertDiskTierSpecificStats(client, "index", 2 + diskReachesSoFar, tookTimeSoFar, -1); - // A final request for something in neither tier shouldn't increment disk specific stats + // A final request for something in neither tier shouldn't increment disk specific stats (once keystore is on) resp = client.prepareSearch("index").setRequestCache(true).setQuery(QueryBuilders.termQuery("k", "hello" + numRequests)).get(); IndicesRequestCacheIT.assertCacheState(client, "index", 0, numRequests + 3, TierType.ON_HEAP, false); IndicesRequestCacheIT.assertCacheState(client, "index", 2, numRequests + 1, TierType.DISK, false); - assertDiskTierSpecificStats(client, "index", 2, tookTimeSoFar, tookTimeSoFar); + //assertDiskTierSpecificStats(client, "index", 2 + diskReachesSoFar, tookTimeSoFar, tookTimeSoFar); + // TODO: Uncomment once keystore is integrated } @@ -120,6 +128,15 @@ private long getCacheSizeBytes(Client client, String index, TierType tierType) { private long assertDiskTierSpecificStats(Client client, String index, long totalDiskReaches, long totalGetTimeLowerBound, long totalGetTimeUpperBound) { // set bounds to -1 to ignore them + ShardRequestCache.DiskStatsAccumulator specStats = getDiskStatsAccumulator(client, index); + assertEquals(totalDiskReaches, specStats.getTotalDiskReaches()); + long tookTime = specStats.getTotalGetTime(); + assertTrue(tookTime >= totalGetTimeLowerBound || totalGetTimeLowerBound < 0); + assertTrue(tookTime <= totalGetTimeUpperBound || totalGetTimeUpperBound < 0); + return tookTime; // Return for use in next check + } + + private ShardRequestCache.DiskStatsAccumulator getDiskStatsAccumulator(Client client, String index) { RequestCacheStats requestCacheStats = client.admin() .indices() .prepareStats(index) @@ -127,11 +144,6 @@ private long assertDiskTierSpecificStats(Client client, String index, long total .get() .getTotal() .getRequestCache(); - ShardRequestCache.DiskStatsAccumulator specStats = requestCacheStats.getDiskSpecificStats(); - assertEquals(totalDiskReaches, specStats.getTotalDiskReaches()); - long tookTime = specStats.getTotalGetTime(); - assertTrue(tookTime >= totalGetTimeLowerBound || totalGetTimeLowerBound < 0); - assertTrue(tookTime <= totalGetTimeUpperBound || totalGetTimeUpperBound < 0); - return tookTime; // Return for use in next check + return requestCacheStats.getDiskSpecificStats(); } } diff --git a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java index 57bfd2a753273..668d869ec99c4 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java @@ -124,6 +124,7 @@ private EhCacheDiskCachingTier(Builder builder) { close(); cacheManager = buildCacheManager(); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); + int i = 0; } private PersistentCacheManager buildCacheManager() { diff --git a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java index 5d4dce28db1ca..ae33c0a90a7e5 100644 --- a/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java +++ b/server/src/main/java/org/opensearch/indices/AbstractIndexShardCacheEntity.java @@ -54,12 +54,11 @@ abstract class AbstractIndexShardCacheEntity implements IndicesRequestCache.Cach @Override public final void onCached(IndicesRequestCache.Key key, BytesReference value, TierType tierType) { - // TODO: Handle tierType in stats - stats().onCached(key, value); + stats().onCached(key, value, tierType); } @Override - final void onHit(CacheValue cacheValue) { + public final void onHit(CacheValue cacheValue) { stats().onHit(cacheValue); } @@ -71,6 +70,11 @@ public final void onMiss(CacheValue cacheValue) { @Override public final void onRemoval(RemovalNotification notification) { // TODO: Handle tierType in stats - stats().onRemoval(notification.getKey(), notification.getValue(), notification.getRemovalReason() == RemovalReason.EVICTED); + stats().onRemoval( + notification.getKey(), + notification.getValue(), + notification.getRemovalReason() == RemovalReason.EVICTED, + notification.getTierType() + ); } } diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 4c7911c89a465..a44339c15ff80 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -40,6 +40,7 @@ import org.apache.lucene.util.RamUsageEstimator; import org.opensearch.common.CheckedSupplier; import org.opensearch.common.cache.RemovalNotification; +import org.opensearch.common.cache.tier.BytesReferenceSerializer; import org.opensearch.common.cache.tier.CacheValue; import org.opensearch.common.cache.tier.EhCacheDiskCachingTier; import org.opensearch.common.cache.tier.OnHeapCachingTier; @@ -128,28 +129,31 @@ public final class IndicesRequestCache implements TieredCacheEventListener k.ramBytesUsed() + v.ramBytesUsed() ).setMaximumWeight(sizeInBytes).setExpireAfterAccess(expire).build(); - // enabling this for testing purposes. Remove/tweak!! - //long CACHE_SIZE_IN_BYTES = 1000000L; - //String SETTING_PREFIX = "indices.request.cache"; + // TODO: Enable/disable switch for disk tier, in cluster settings PR + long CACHE_SIZE_IN_BYTES = 10000000L; // Set to 10 MB for now, will be changed in cluster settings PR + String SETTING_PREFIX = "indices.request.cache"; + String STORAGE_PATH = indicesService.getNodePaths()[0].indicesPath.toString() + "/request_cache"; - /*EhCacheDiskCachingTier ehcacheDiskTier = new EhCacheDiskCachingTier.Builder() + EhCacheDiskCachingTier ehcacheDiskTier = new EhCacheDiskCachingTier.Builder() .setKeyType(Key.class) .setValueType(BytesReference.class) .setExpireAfterAccess(TimeValue.MAX_VALUE) .setSettings(settings) .setThreadPoolAlias("ehcacheTest") .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) - .setStoragePath("/tmp") + .setStoragePath(STORAGE_PATH) .setSettingPrefix(SETTING_PREFIX) - .build();*/ + .setKeySerializer(new IRCKeyWriteableSerializer(this)) + .setValueSerializer(new BytesReferenceSerializer()) + .build(); // Initialize tiered cache service. TODO: Enable Disk tier when tiered support is turned on. - tieredCacheService = new TieredCacheSpilloverStrategyService.Builder().setOnHeapCachingTier( - openSearchOnHeapCache - ) - //.setOnDiskCachingTier(ehcacheDiskTier) - .setTieredCacheEventListener(this).build(); + tieredCacheService = new TieredCacheSpilloverStrategyService.Builder() + .setOnHeapCachingTier(openSearchOnHeapCache) + .setOnDiskCachingTier(ehcacheDiskTier) + .setTieredCacheEventListener(this) + .build(); } @Override From d64cd2616344c8f651e9d08c2a75e1c25740704d Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 3 Jan 2024 09:54:42 -0800 Subject: [PATCH 17/18] attempt to fix IT bug Signed-off-by: Peter Alfonsi --- .../indices/IndicesRequestCacheIT.java | 1 - .../indices/IndicesRequestCache.java | 63 +++++++++++-------- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index bdec4283c368a..9680f407d9aaa 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -44,7 +44,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/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index a44339c15ff80..cb1c94fbd7e29 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -118,42 +118,31 @@ public final class IndicesRequestCache implements TieredCacheEventListener tieredCacheService; private final IndicesService indicesService; + private final Settings settings; + IndicesRequestCache(Settings settings, IndicesService indicesService) { this.size = INDICES_CACHE_QUERY_SIZE.get(settings); this.expire = INDICES_CACHE_QUERY_EXPIRE.exists(settings) ? INDICES_CACHE_QUERY_EXPIRE.get(settings) : null; long sizeInBytes = size.getBytes(); this.indicesService = indicesService; + this.settings = settings; // Initialize onHeap cache tier first. OnHeapCachingTier openSearchOnHeapCache = new OpenSearchOnHeapCache.Builder().setWeigher( (k, v) -> k.ramBytesUsed() + v.ramBytesUsed() ).setMaximumWeight(sizeInBytes).setExpireAfterAccess(expire).build(); - // TODO: Enable/disable switch for disk tier, in cluster settings PR - long CACHE_SIZE_IN_BYTES = 10000000L; // Set to 10 MB for now, will be changed in cluster settings PR - String SETTING_PREFIX = "indices.request.cache"; - String STORAGE_PATH = indicesService.getNodePaths()[0].indicesPath.toString() + "/request_cache"; - EhCacheDiskCachingTier ehcacheDiskTier = new EhCacheDiskCachingTier.Builder() - .setKeyType(Key.class) - .setValueType(BytesReference.class) - .setExpireAfterAccess(TimeValue.MAX_VALUE) - .setSettings(settings) - .setThreadPoolAlias("ehcacheTest") - .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) - .setStoragePath(STORAGE_PATH) - .setSettingPrefix(SETTING_PREFIX) - .setKeySerializer(new IRCKeyWriteableSerializer(this)) - .setValueSerializer(new BytesReferenceSerializer()) - .build(); + // Initialize tiered cache service. + TieredCacheSpilloverStrategyService.Builder tieredCacheServiceBuilder = + new TieredCacheSpilloverStrategyService.Builder() + .setOnHeapCachingTier(openSearchOnHeapCache) + .setTieredCacheEventListener(this); - // Initialize tiered cache service. TODO: Enable Disk tier when tiered support is turned on. - tieredCacheService = new TieredCacheSpilloverStrategyService.Builder() - .setOnHeapCachingTier(openSearchOnHeapCache) - .setOnDiskCachingTier(ehcacheDiskTier) - .setTieredCacheEventListener(this) - .build(); + EhCacheDiskCachingTier ehcacheDiskTier = createNewDiskTier(); + tieredCacheServiceBuilder.setOnDiskCachingTier(ehcacheDiskTier); + tieredCacheService = tieredCacheServiceBuilder.build(); } @Override @@ -215,9 +204,6 @@ BytesReference getOrCompute( } } } - // else { - // key.entity.onHit(); - // } return value; } @@ -243,7 +229,6 @@ void invalidate(CacheEntity cacheEntity, DirectoryReader reader, BytesReference * @opensearch.internal */ private static class Loader implements TieredCacheLoader { - private final CacheEntity entity; private final CheckedSupplier loader; private boolean loaded; @@ -309,7 +294,7 @@ interface CacheEntity extends Accountable, Writeable { * * @opensearch.internal */ - class Key implements Accountable, Writeable { + class Key implements Accountable, Writeable { private final long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(Key.class); public final CacheEntity entity; // use as identity equality @@ -446,4 +431,28 @@ long count() { int numRegisteredCloseListeners() { // for testing return registeredClosedListeners.size(); } + + /** + * Creates a new disk tier instance. Should only be run if the instance will actually be used! + * Otherwise, it may not be closed properly. + * @return A new disk tier instance + */ + public EhCacheDiskCachingTier createNewDiskTier() { + //assert FeatureFlags.isEnabled(FeatureFlags.TIERED_CACHING); + long CACHE_SIZE_IN_BYTES = 10000000L; //INDICES_CACHE_DISK_TIER_SIZE.get(settings).getBytes(); + String STORAGE_PATH = indicesService.getNodePaths()[0].indicesPath.toString() + "/request_cache"; + + return new EhCacheDiskCachingTier.Builder() + .setKeyType(Key.class) + .setValueType(BytesReference.class) + .setExpireAfterAccess(TimeValue.MAX_VALUE) // TODO: Is this meant to be the same as IRC expire or different? + .setThreadPoolAlias("ehcacheThreadpool") + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setStoragePath(STORAGE_PATH) + .setKeySerializer(new IRCKeyWriteableSerializer(this)) + .setValueSerializer(new BytesReferenceSerializer()) + .setSettings(settings) + .setSettingPrefix("indices.requests.tier") + .build(); + } } From 52cb67e348bbfbeb584da63afb55aee0d07e9cec Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 3 Jan 2024 13:42:41 -0800 Subject: [PATCH 18/18] Fixed issue with IRC UTs Signed-off-by: Peter Alfonsi --- .../common/cache/tier/EhCacheDiskCachingTier.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java index 668d869ec99c4..e1c9d972b6d1a 100644 --- a/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java +++ b/server/src/main/java/org/opensearch/common/cache/tier/EhCacheDiskCachingTier.java @@ -11,6 +11,7 @@ import org.ehcache.core.spi.service.FileBasedPersistenceContext; import org.ehcache.spi.serialization.SerializerException; import org.opensearch.OpenSearchException; +import org.opensearch.common.Randomness; import org.opensearch.common.cache.RemovalListener; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; @@ -26,6 +27,8 @@ import java.util.NoSuchElementException; import java.util.Objects; import java.util.Optional; +import java.util.Random; +import java.util.UUID; import java.util.function.Supplier; import org.ehcache.Cache; @@ -50,7 +53,7 @@ public class EhCacheDiskCachingTier implements DiskCachingTier { // A Cache manager can create many caches. - private static PersistentCacheManager cacheManager = null; + private PersistentCacheManager cacheManager; // Disk cache private Cache cache; @@ -104,7 +107,7 @@ private EhCacheDiskCachingTier(Builder builder) { this.valueSerializer = Objects.requireNonNull(builder.valueSerializer, "Value serializer shouldn't be null"); this.ehCacheEventListener = new EhCacheEventListener(this.valueSerializer); this.maxWeightInBytes = builder.maxWeightInBytes; - this.storagePath = Objects.requireNonNull(builder.storagePath, "Storage path shouldn't be null"); + this.storagePath = Objects.requireNonNull(builder.storagePath, "Storage path shouldn't be null") + UUID.randomUUID(); // temporary fix if (builder.threadPoolAlias == null || builder.threadPoolAlias.isBlank()) { this.threadPoolAlias = THREAD_POOL_ALIAS_PREFIX + "DiskWrite"; } else { @@ -119,12 +122,8 @@ private EhCacheDiskCachingTier(Builder builder) { // Default value is 16 within Ehcache. this.DISK_SEGMENTS = Setting.intSetting(builder.settingPrefix + ".ehcache.disk.segments", 16, 1, 32); - // In test cases, there might be leftover cache managers and caches hanging around, from nodes created in the test case setup - // Destroy them before recreating them - close(); cacheManager = buildCacheManager(); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); - int i = 0; } private PersistentCacheManager buildCacheManager() {