From 2ac64a6262973383d71f3e3ee037e4d267c3b592 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 19 Nov 2024 09:15:09 -0800 Subject: [PATCH] Changing request cache size > 0 setting to int threshold (#16570) Signed-off-by: Peter Alfonsi Co-authored-by: Peter Alfonsi --- .../indices/IndicesRequestCacheIT.java | 20 +++++++++++++++---- .../common/settings/ClusterSettings.java | 2 +- .../indices/IndicesRequestCache.java | 16 +++++++++------ .../opensearch/indices/IndicesService.java | 17 ++++++++-------- .../indices/IndicesServiceTests.java | 13 ++++++------ 5 files changed, 42 insertions(+), 26 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index bab085bf265af..a16d2065598ba 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -90,7 +90,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING; -import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING; +import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING; import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.search.aggregations.AggregationBuilders.dateHistogram; import static org.opensearch.search.aggregations.AggregationBuilders.dateRange; @@ -582,21 +582,33 @@ public void testCanCache() throws Exception { assertThat(r4.getHits().getTotalHits().value, equalTo(7L)); assertCacheState(client, index, 0, 4); - // If size > 0 we should cache if this is enabled via cluster setting + // Update max cacheable size for request cache from default value of 0 ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest(); + int maxCacheableSize = 5; updateSettingsRequest.persistentSettings( - Settings.builder().put(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true) + Settings.builder().put(INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING.getKey(), maxCacheableSize) ); assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); + // Sizes <= the cluster setting value should be cached final SearchResponse r7 = client.prepareSearch(index) .setSearchType(SearchType.QUERY_THEN_FETCH) - .setSize(1) + .setSize(maxCacheableSize) .setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-22").lte("2016-03-26")) .get(); OpenSearchAssertions.assertAllSuccessful(r7); assertThat(r7.getHits().getTotalHits().value, equalTo(5L)); assertCacheState(client, index, 0, 6); + + // Sizes > the cluster setting value should not be cached + final SearchResponse r8 = client.prepareSearch(index) + .setSearchType(SearchType.QUERY_THEN_FETCH) + .setSize(maxCacheableSize + 1) + .setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-22").lte("2016-03-26")) + .get(); + OpenSearchAssertions.assertAllSuccessful(r8); + assertThat(r8.getHits().getTotalHits().value, equalTo(5L)); + assertCacheState(client, index, 0, 6); } public void testCacheWithFilteredAlias() throws InterruptedException { diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index c836984655ad1..04a19e32c4ebc 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -519,7 +519,7 @@ public void apply(Settings value, Settings current, Settings previous) { IndicesRequestCache.INDICES_CACHE_QUERY_EXPIRE, IndicesRequestCache.INDICES_REQUEST_CACHE_CLEANUP_INTERVAL_SETTING, IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING, - IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING, + IndicesRequestCache.INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING, HunspellService.HUNSPELL_LAZY_LOAD, HunspellService.HUNSPELL_IGNORE_CASE, HunspellService.HUNSPELL_DICTIONARY_OPTIONS, diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 4dde4445cd483..3d158cb60a208 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -148,13 +148,17 @@ public final class IndicesRequestCache implements RemovalListener 0 queries. - * If enabled, fundamentally non-cacheable queries like DFS queries, queries using the `now` keyword, and - * scroll requests are still not cached. + * Sets the maximum size of a query which is allowed in the request cache. + * This refers to the number of documents returned, not the size in bytes. + * Default value of 0 only allows size == 0 queries, matching earlier behavior. + * Fundamentally non-cacheable queries like DFS queries, queries using the `now` keyword, and + * scroll requests are never cached, regardless of this setting. */ - public static final Setting INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting( - "indices.requests.cache.enable_for_all_requests", - false, + public static final Setting INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING = Setting.intSetting( + "indices.requests.cache.maximum_cacheable_size", + 0, + 0, + 10_000, Property.NodeScope, Property.Dynamic ); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 1a4c9067939a9..b9bad5527e3f4 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -205,7 +205,7 @@ import static org.opensearch.index.IndexService.IndexCreationContext.CREATE_INDEX; import static org.opensearch.index.IndexService.IndexCreationContext.METADATA_VERIFICATION; import static org.opensearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder; -import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING; +import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.isRemoteDataAttributePresent; import static org.opensearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES; @@ -361,7 +361,7 @@ public class IndicesService extends AbstractLifecycleComponent private final FileCache fileCache; private final CompositeIndexSettings compositeIndexSettings; private final Consumer replicator; - private volatile boolean requestCachingEnabledForAllQueries; + private volatile int maxSizeInRequestCache; @Override protected void doStart() { @@ -509,9 +509,9 @@ protected void closeInternal() { this.compositeIndexSettings = compositeIndexSettings; this.fileCache = fileCache; this.replicator = replicator; - this.requestCachingEnabledForAllQueries = INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.get(clusterService.getSettings()); + this.maxSizeInRequestCache = INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING.get(clusterService.getSettings()); clusterService.getClusterSettings() - .addSettingsUpdateConsumer(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING, this::setRequestCachingEnabledForAllQueries); + .addSettingsUpdateConsumer(INDICES_REQUEST_CACHE_MAX_SIZE_ALLOWED_IN_CACHE_SETTING, this::setMaxSizeInRequestCache); } public IndicesService( @@ -1752,10 +1752,9 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) { // if not explicitly set in the request, use the index setting, if not, use the request if (request.requestCache() == null) { if (settings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING) == false - || (context.size() > 0 && !requestCachingEnabledForAllQueries)) { + || (context.size() > maxSizeInRequestCache)) { // If no request cache query parameter and shard request cache - // is enabled in settings don't cache for requests with size > 0 - // unless this is enabled via cluster setting + // is enabled in settings, use cluster setting to check the maximum size allowed in the cache return false; } } else if (request.requestCache() == false) { @@ -2125,7 +2124,7 @@ public CompositeIndexSettings getCompositeIndexSettings() { } // Package-private for testing - void setRequestCachingEnabledForAllQueries(Boolean requestCachingEnabledForAllQueries) { - this.requestCachingEnabledForAllQueries = requestCachingEnabledForAllQueries; + void setMaxSizeInRequestCache(Integer maxSizeInRequestCache) { + this.maxSizeInRequestCache = maxSizeInRequestCache; } } diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java index d2250702b48fd..9c717c796daae 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java @@ -652,9 +652,7 @@ public void testDirectoryReaderWithoutDelegatingCacheHelperNotCacheable() throws } public void testCanCacheSizeNonzero() { - // Size == 0 requests should always be cacheable (if they pass the other checks). - // Size > 0 requests should only be cacheable if ALLOW_SIZE_NONZERO_SETTING is true. - + // Requests should only be cached if their size is <= INDICES_REQUEST_CACHE_MAX_SIZE_TO_CACHE_SETTING. final IndexService indexService = createIndex("test"); ShardSearchRequest request = mock(ShardSearchRequest.class); when(request.requestCache()).thenReturn(null); @@ -662,7 +660,7 @@ public void testCanCacheSizeNonzero() { TestSearchContext sizeZeroContext = getTestContext(indexService, 0); TestSearchContext sizeNonzeroContext = getTestContext(indexService, 10); - // Test for an IndicesService with the default setting value of false + // Test for an IndicesService with the default setting value of 0 IndicesService indicesService = getIndicesService(); DelegatingCacheHelper cacheHelper = mock(DelegatingCacheHelper.class); Map expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, false); @@ -673,8 +671,11 @@ public void testCanCacheSizeNonzero() { assertEquals(entry.getValue(), indicesService.canCache(request, context)); } // Simulate the cluster setting update by manually calling setCanCacheSizeNonzeroRequests - indicesService.setRequestCachingEnabledForAllQueries(true); - expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, true); + int maxCacheableSize = 40; + indicesService.setMaxSizeInRequestCache(maxCacheableSize); + TestSearchContext sizeEqualsThresholdContext = getTestContext(indexService, maxCacheableSize); + TestSearchContext sizeAboveThresholdContext = getTestContext(indexService, maxCacheableSize + 5); + expectedResultMap = Map.of(sizeZeroContext, true, sizeEqualsThresholdContext, true, sizeAboveThresholdContext, false); for (Map.Entry entry : expectedResultMap.entrySet()) { TestSearchContext context = entry.getKey();