Changing request cache size > 0 setting to int threshold (opensearch-…

…project#16570) Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
andrross · Nov 19, 2024 · 2ac64a6 · 2ac64a6
1 parent 1d8568e
commit 2ac64a6
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 26 deletions.
diff --git 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
@@ -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
@@ -148,13 +148,17 @@ public final class IndicesRequestCache implements RemovalListener<ICacheKey<Indi
     );
 
     /**
-     * If enabled, allows caching all cacheable queries. For now, this means also allowing size > 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<Boolean> INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting(
-        "indices.requests.cache.enable_for_all_requests",
-        false,
+    public static final Setting<Integer> 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
@@ -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<IndexShard> 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
@@ -652,17 +652,15 @@ 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);
 
         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<TestSearchContext, Boolean> 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<TestSearchContext, Boolean> entry : expectedResultMap.entrySet()) {
             TestSearchContext context = entry.getKey();