From 29a23c03a73221b2471499aecc8152f9ffa7cc9e Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 25 Oct 2024 10:13:28 -0700 Subject: [PATCH 1/9] Add cluster setting to allow size>0 in request cache Signed-off-by: Peter Alfonsi --- .../indices/IndicesRequestCacheIT.java | 16 +++++ .../common/settings/ClusterSettings.java | 1 + .../indices/IndicesRequestCache.java | 10 +++ .../opensearch/indices/IndicesService.java | 12 +++- .../indices/IndicesServiceTests.java | 69 +++++++++++++++---- 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 557f9e19ee424..d38a415403cca 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -43,6 +43,7 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.action.admin.cluster.node.stats.NodeStats; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; +import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.opensearch.action.admin.indices.alias.Alias; import org.opensearch.action.admin.indices.cache.clear.ClearIndicesCacheRequest; import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; @@ -89,6 +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.ALLOW_SIZE_NONZERO_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; @@ -579,6 +581,20 @@ public void testCanCache() throws Exception { OpenSearchAssertions.assertAllSuccessful(r4); 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 + ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest(); + updateSettingsRequest.persistentSettings(Settings.builder().put(ALLOW_SIZE_NONZERO_SETTING.getKey(), true)); + assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); + + final SearchResponse r7 = client.prepareSearch(index) + .setSearchType(SearchType.QUERY_THEN_FETCH) + .setSize(1) + .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); } 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 f769f8729c25b..346fb53386d39 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -518,6 +518,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.ALLOW_SIZE_NONZERO_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 156fe32ff5809..160181efe98c4 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -147,6 +147,16 @@ public final class IndicesRequestCache implements RemovalListener 0 queries. + */ + public static final Setting ALLOW_SIZE_NONZERO_SETTING = Setting.boolSetting( + "indices.requests.cache.allow_size_nonzero", + false, + Property.NodeScope, + Property.Dynamic + ); + private final static long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(Key.class); private final ConcurrentMap registeredClosedListeners = ConcurrentCollections.newConcurrentMap(); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 1c12e8ca17194..9455e24e0de74 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -205,6 +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.ALLOW_SIZE_NONZERO_SETTING; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.isRemoteDataAttributePresent; import static org.opensearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES; @@ -360,6 +361,7 @@ public class IndicesService extends AbstractLifecycleComponent private final FileCache fileCache; private final CompositeIndexSettings compositeIndexSettings; private final Consumer replicator; + private boolean canCacheSizeNonzeroRequests; @Override protected void doStart() { @@ -507,6 +509,8 @@ protected void closeInternal() { this.compositeIndexSettings = compositeIndexSettings; this.fileCache = fileCache; this.replicator = replicator; + this.canCacheSizeNonzeroRequests = ALLOW_SIZE_NONZERO_SETTING.get(clusterService.getSettings()); + clusterService.getClusterSettings().addSettingsUpdateConsumer(ALLOW_SIZE_NONZERO_SETTING, this::setCanCacheSizeNonzeroRequests); } public IndicesService( @@ -1748,9 +1752,10 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) { if (request.requestCache() == null) { if (settings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING) == false) { return false; - } else if (context.size() != 0) { + } else if (context.size() != 0 && !canCacheSizeNonzeroRequests) { // 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 return false; } } else if (request.requestCache() == false) { @@ -2118,4 +2123,9 @@ public RemoteStoreSettings getRemoteStoreSettings() { public CompositeIndexSettings getCompositeIndexSettings() { return this.compositeIndexSettings; } + + // Package-private for testing + void setCanCacheSizeNonzeroRequests(Boolean canCacheSizeNonzeroRequests) { + this.canCacheSizeNonzeroRequests = canCacheSizeNonzeroRequests; + } } diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java index b5350a39e8599..05b25586915b0 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java @@ -641,25 +641,68 @@ public void testDirectoryReaderWithoutDelegatingCacheHelperNotCacheable() throws ShardSearchRequest request = mock(ShardSearchRequest.class); when(request.requestCache()).thenReturn(true); - TestSearchContext context = new TestSearchContext(indexService.getBigArrays(), indexService) { - @Override - public SearchType searchType() { - return SearchType.QUERY_THEN_FETCH; - } - }; + TestSearchContext context = getTestContext(indexService, 0); + IndexReader.CacheHelper notDelegatingCacheHelper = mock(IndexReader.CacheHelper.class); + DelegatingCacheHelper delegatingCacheHelper = mock(DelegatingCacheHelper.class); + for (boolean useDelegatingCacheHelper : new boolean[] { true, false }) { + IndexReader.CacheHelper cacheHelper = useDelegatingCacheHelper ? delegatingCacheHelper : notDelegatingCacheHelper; + setupMocksForCanCache(context, cacheHelper); + assertEquals(useDelegatingCacheHelper, indicesService.canCache(request, context)); + } + } + + 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. + + 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 + IndicesService indicesService = getIndicesService(); + DelegatingCacheHelper cacheHelper = mock(DelegatingCacheHelper.class); + Map expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, false); + for (Map.Entry entry : expectedResultMap.entrySet()) { + TestSearchContext context = entry.getKey(); + setupMocksForCanCache(context, cacheHelper); + assertEquals(entry.getValue(), indicesService.canCache(request, context)); + } + // Simulate the cluster setting update by manually calling setCanCacheSizeNonzeroRequests + indicesService.setCanCacheSizeNonzeroRequests(true); + expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, true); + + for (Map.Entry entry : expectedResultMap.entrySet()) { + TestSearchContext context = entry.getKey(); + setupMocksForCanCache(context, cacheHelper); + assertEquals(entry.getValue(), indicesService.canCache(request, context)); + } + } + + private void setupMocksForCanCache(TestSearchContext context, IndexReader.CacheHelper cacheHelper) { ContextIndexSearcher searcher = mock(ContextIndexSearcher.class); context.setSearcher(searcher); DirectoryReader reader = mock(DirectoryReader.class); when(searcher.getDirectoryReader()).thenReturn(reader); when(searcher.getIndexReader()).thenReturn(reader); - IndexReader.CacheHelper notDelegatingCacheHelper = mock(IndexReader.CacheHelper.class); - DelegatingCacheHelper delegatingCacheHelper = mock(DelegatingCacheHelper.class); + when(reader.getReaderCacheHelper()).thenReturn(cacheHelper); + } - for (boolean useDelegatingCacheHelper : new boolean[] { true, false }) { - IndexReader.CacheHelper cacheHelper = useDelegatingCacheHelper ? delegatingCacheHelper : notDelegatingCacheHelper; - when(reader.getReaderCacheHelper()).thenReturn(cacheHelper); - assertEquals(useDelegatingCacheHelper, indicesService.canCache(request, context)); - } + private TestSearchContext getTestContext(IndexService indexService, int size) { + return new TestSearchContext(indexService.getBigArrays(), indexService) { + @Override + public SearchType searchType() { + return SearchType.QUERY_THEN_FETCH; + } + + @Override + public int size() { + return size; + } + }; } } From ccfb3e47f2f7cabda263116e6541e9d95c7b036e Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 25 Oct 2024 11:29:54 -0700 Subject: [PATCH 2/9] Add to changelog Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e682c1b226f4a..b0f32ffc7cb66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - URI path filtering support in cluster stats API ([#15938](https://github.com/opensearch-project/OpenSearch/pull/15938)) - [Star Tree - Search] Add support for metric aggregations with/without term query ([15289](https://github.com/opensearch-project/OpenSearch/pull/15289)) - Add support for restoring from snapshot with search replicas ([#16111](https://github.com/opensearch-project/OpenSearch/pull/16111)) +- Add dynamic setting allowing size > 0 requests to be cached in the request cache ([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483/files)) ### Dependencies - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578)) From 86ec73e6f0469bb03c31e85aba7447cfa8f2acf6 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 09:07:09 -0700 Subject: [PATCH 3/9] addressed dbwiddis's comments Signed-off-by: Peter Alfonsi --- .../src/main/java/org/opensearch/indices/IndicesService.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 9455e24e0de74..3762694bec3ce 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -1750,9 +1750,8 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) { IndexSettings settings = context.indexShard().indexSettings(); // 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) { - return false; - } else if (context.size() != 0 && !canCacheSizeNonzeroRequests) { + if (settings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING) == false + || (context.size() > 0 && !canCacheSizeNonzeroRequests)) { // 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 From 3258000b79092cc669f1a335197e8d359368f21f Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 11:51:44 -0700 Subject: [PATCH 4/9] make canCacheSizeNonzeroRequests volatile Signed-off-by: Peter Alfonsi --- server/src/main/java/org/opensearch/indices/IndicesService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 3762694bec3ce..c3c9f6d204a8e 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -361,7 +361,7 @@ public class IndicesService extends AbstractLifecycleComponent private final FileCache fileCache; private final CompositeIndexSettings compositeIndexSettings; private final Consumer replicator; - private boolean canCacheSizeNonzeroRequests; + private volatile boolean canCacheSizeNonzeroRequests; @Override protected void doStart() { From f54a0cfe507abb00548bb08290e52fdd22b2c3f5 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 11:53:28 -0700 Subject: [PATCH 5/9] fix changelog merge Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e15371cf0ba98..c2923d3ae1328 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add logic in master service to optimize performance and retain detailed logging for critical cluster operations. ([#14795](https://github.com/opensearch-project/OpenSearch/pull/14795)) - Add Setting to adjust the primary constraint weights ([#16471](https://github.com/opensearch-project/OpenSearch/pull/16471)) - Switch from `buildSrc/version.properties` to Gradle version catalog (`gradle/libs.versions.toml`) to enable dependabot to perform automated upgrades on common libs ([#16284](https://github.com/opensearch-project/OpenSearch/pull/16284)) -- - Add dynamic setting allowing size > 0 requests to be cached in the request cache ([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483/files)) +- Add dynamic setting allowing size > 0 requests to be cached in the request cache ([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483/files)) ### Dependencies - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578)) From 1d955ef2344d61ad7d3fbe14e21ab66cd0e0c76d Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 14:32:09 -0700 Subject: [PATCH 6/9] Changed setting name Signed-off-by: Peter Alfonsi --- .../opensearch/indices/IndicesRequestCacheIT.java | 4 ++-- .../common/settings/ClusterSettings.java | 2 +- .../opensearch/indices/IndicesRequestCache.java | 8 +++++--- .../org/opensearch/indices/IndicesService.java | 15 ++++++++------- .../opensearch/indices/IndicesServiceTests.java | 2 +- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index d38a415403cca..856368aaaf8cb 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.ALLOW_SIZE_NONZERO_SETTING; +import static org.opensearch.indices.IndicesRequestCache.ENABLE_FOR_ALL_REQUESTS_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; @@ -584,7 +584,7 @@ public void testCanCache() throws Exception { // If size > 0 we should cache if this is enabled via cluster setting ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest(); - updateSettingsRequest.persistentSettings(Settings.builder().put(ALLOW_SIZE_NONZERO_SETTING.getKey(), true)); + updateSettingsRequest.persistentSettings(Settings.builder().put(ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true)); assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); final SearchResponse r7 = client.prepareSearch(index) 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 c9df7a2266263..cde9cde009950 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.ALLOW_SIZE_NONZERO_SETTING, + IndicesRequestCache.ENABLE_FOR_ALL_REQUESTS_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 160181efe98c4..c88fb3f99c1a8 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -148,10 +148,12 @@ public final class IndicesRequestCache implements RemovalListener 0 queries. + * 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. */ - public static final Setting ALLOW_SIZE_NONZERO_SETTING = Setting.boolSetting( - "indices.requests.cache.allow_size_nonzero", + public static final Setting ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting( + "indices.requests.cache.enable_for_all_requests", false, 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 c3c9f6d204a8e..da10f158e3adf 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.ALLOW_SIZE_NONZERO_SETTING; +import static org.opensearch.indices.IndicesRequestCache.ENABLE_FOR_ALL_REQUESTS_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 canCacheSizeNonzeroRequests; + private volatile boolean cachingEnabledForAllQueries; @Override protected void doStart() { @@ -509,8 +509,9 @@ protected void closeInternal() { this.compositeIndexSettings = compositeIndexSettings; this.fileCache = fileCache; this.replicator = replicator; - this.canCacheSizeNonzeroRequests = ALLOW_SIZE_NONZERO_SETTING.get(clusterService.getSettings()); - clusterService.getClusterSettings().addSettingsUpdateConsumer(ALLOW_SIZE_NONZERO_SETTING, this::setCanCacheSizeNonzeroRequests); + this.cachingEnabledForAllQueries = ENABLE_FOR_ALL_REQUESTS_SETTING.get(clusterService.getSettings()); + clusterService.getClusterSettings() + .addSettingsUpdateConsumer(ENABLE_FOR_ALL_REQUESTS_SETTING, this::setCachingEnabledForAllQueries); } public IndicesService( @@ -1751,7 +1752,7 @@ 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 && !canCacheSizeNonzeroRequests)) { + || (context.size() > 0 && !cachingEnabledForAllQueries)) { // 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 @@ -2124,7 +2125,7 @@ public CompositeIndexSettings getCompositeIndexSettings() { } // Package-private for testing - void setCanCacheSizeNonzeroRequests(Boolean canCacheSizeNonzeroRequests) { - this.canCacheSizeNonzeroRequests = canCacheSizeNonzeroRequests; + void setCachingEnabledForAllQueries(Boolean cachingEnabledForAllQueries) { + this.cachingEnabledForAllQueries = cachingEnabledForAllQueries; } } diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java index 05b25586915b0..8c019b4065cac 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java @@ -673,7 +673,7 @@ public void testCanCacheSizeNonzero() { assertEquals(entry.getValue(), indicesService.canCache(request, context)); } // Simulate the cluster setting update by manually calling setCanCacheSizeNonzeroRequests - indicesService.setCanCacheSizeNonzeroRequests(true); + indicesService.setCachingEnabledForAllQueries(true); expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, true); for (Map.Entry entry : expectedResultMap.entrySet()) { From fe3818a95842d5a72cdd0a6c1b15188c1b299baa Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 16:13:20 -0700 Subject: [PATCH 7/9] more renaming Signed-off-by: Peter Alfonsi --- .../opensearch/indices/IndicesRequestCacheIT.java | 4 ++-- .../common/settings/ClusterSettings.java | 2 +- .../opensearch/indices/IndicesRequestCache.java | 2 +- .../org/opensearch/indices/IndicesService.java | 14 +++++++------- .../opensearch/indices/IndicesServiceTests.java | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index 856368aaaf8cb..d34749ca53486 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.ENABLE_FOR_ALL_REQUESTS_SETTING; +import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_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; @@ -584,7 +584,7 @@ public void testCanCache() throws Exception { // If size > 0 we should cache if this is enabled via cluster setting ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest(); - updateSettingsRequest.persistentSettings(Settings.builder().put(ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true)); + updateSettingsRequest.persistentSettings(Settings.builder().put(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true)); assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); final SearchResponse r7 = client.prepareSearch(index) 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 cde9cde009950..cac4b3914df5a 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.ENABLE_FOR_ALL_REQUESTS_SETTING, + IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_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 c88fb3f99c1a8..4dde4445cd483 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -152,7 +152,7 @@ public final class IndicesRequestCache implements RemovalListener ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting( + public static final Setting INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting( "indices.requests.cache.enable_for_all_requests", false, Property.NodeScope, diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index da10f158e3adf..1a4c9067939a9 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.ENABLE_FOR_ALL_REQUESTS_SETTING; +import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_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 cachingEnabledForAllQueries; + private volatile boolean requestCachingEnabledForAllQueries; @Override protected void doStart() { @@ -509,9 +509,9 @@ protected void closeInternal() { this.compositeIndexSettings = compositeIndexSettings; this.fileCache = fileCache; this.replicator = replicator; - this.cachingEnabledForAllQueries = ENABLE_FOR_ALL_REQUESTS_SETTING.get(clusterService.getSettings()); + this.requestCachingEnabledForAllQueries = INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.get(clusterService.getSettings()); clusterService.getClusterSettings() - .addSettingsUpdateConsumer(ENABLE_FOR_ALL_REQUESTS_SETTING, this::setCachingEnabledForAllQueries); + .addSettingsUpdateConsumer(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING, this::setRequestCachingEnabledForAllQueries); } public IndicesService( @@ -1752,7 +1752,7 @@ 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 && !cachingEnabledForAllQueries)) { + || (context.size() > 0 && !requestCachingEnabledForAllQueries)) { // 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 @@ -2125,7 +2125,7 @@ public CompositeIndexSettings getCompositeIndexSettings() { } // Package-private for testing - void setCachingEnabledForAllQueries(Boolean cachingEnabledForAllQueries) { - this.cachingEnabledForAllQueries = cachingEnabledForAllQueries; + void setRequestCachingEnabledForAllQueries(Boolean requestCachingEnabledForAllQueries) { + this.requestCachingEnabledForAllQueries = requestCachingEnabledForAllQueries; } } diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java index 8c019b4065cac..d2250702b48fd 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceTests.java @@ -673,7 +673,7 @@ public void testCanCacheSizeNonzero() { assertEquals(entry.getValue(), indicesService.canCache(request, context)); } // Simulate the cluster setting update by manually calling setCanCacheSizeNonzeroRequests - indicesService.setCachingEnabledForAllQueries(true); + indicesService.setRequestCachingEnabledForAllQueries(true); expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, true); for (Map.Entry entry : expectedResultMap.entrySet()) { From c37b2bb6226e42264eabf001838e9cf7e1ce6a41 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 28 Oct 2024 16:38:16 -0700 Subject: [PATCH 8/9] fix spotless check Signed-off-by: Peter Alfonsi --- .../java/org/opensearch/indices/IndicesRequestCacheIT.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java index d34749ca53486..bab085bf265af 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java @@ -584,7 +584,9 @@ public void testCanCache() throws Exception { // If size > 0 we should cache if this is enabled via cluster setting ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest(); - updateSettingsRequest.persistentSettings(Settings.builder().put(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true)); + updateSettingsRequest.persistentSettings( + Settings.builder().put(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true) + ); assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet()); final SearchResponse r7 = client.prepareSearch(index) From 99cbb04c7360ce282e4cb93db6e3c316e3947448 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 29 Oct 2024 10:35:15 -0700 Subject: [PATCH 9/9] rerun gradle check Signed-off-by: Peter Alfonsi