Skip to content

Commit 0363aa7

Browse files
peteralfonsiPeter Alfonsi
peteralfonsi
and
Peter Alfonsi
authored
Adds cluster setting to allow caching requests with size>0 in request cache (opensearch-project#16484)
* Add cluster setting to allow size>0 in request cache Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Add to changelog Signed-off-by: Peter Alfonsi <petealft@amazon.com> * addressed dbwiddis's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> * make canCacheSizeNonzeroRequests volatile Signed-off-by: Peter Alfonsi <petealft@amazon.com> * fix changelog merge Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Changed setting name Signed-off-by: Peter Alfonsi <petealft@amazon.com> * more renaming Signed-off-by: Peter Alfonsi <petealft@amazon.com> * fix spotless check Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun gradle check Signed-off-by: Peter Alfonsi <petealft@amazon.com> --------- Signed-off-by: Peter Alfonsi <petealft@amazon.com> Signed-off-by: Peter Alfonsi <peter.alfonsi@gmail.com> Co-authored-by: Peter Alfonsi <petealft@amazon.com>
1 parent 80ca32f commit 0363aa7

File tree

6 files changed

+101
-16
lines changed

6 files changed

+101
-16
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1010
- 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))
1111
- Add Setting to adjust the primary constraint weights ([#16471](https://github.com/opensearch-project/OpenSearch/pull/16471))
1212
- 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))
13+
- Add dynamic setting allowing size > 0 requests to be cached in the request cache ([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483/files))
1314

1415
### Dependencies
1516
- Bump `com.azure:azure-storage-common` from 12.25.1 to 12.27.1 ([#16521](https://github.com/opensearch-project/OpenSearch/pull/16521))

‎server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java

+18
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.opensearch.action.admin.cluster.health.ClusterHealthResponse;
4444
import org.opensearch.action.admin.cluster.node.stats.NodeStats;
4545
import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
46+
import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
4647
import org.opensearch.action.admin.indices.alias.Alias;
4748
import org.opensearch.action.admin.indices.cache.clear.ClearIndicesCacheRequest;
4849
import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse;
@@ -89,6 +90,7 @@
8990
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS;
9091
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
9192
import static org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING;
93+
import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING;
9294
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
9395
import static org.opensearch.search.aggregations.AggregationBuilders.dateHistogram;
9496
import static org.opensearch.search.aggregations.AggregationBuilders.dateRange;
@@ -579,6 +581,22 @@ public void testCanCache() throws Exception {
579581
OpenSearchAssertions.assertAllSuccessful(r4);
580582
assertThat(r4.getHits().getTotalHits().value, equalTo(7L));
581583
assertCacheState(client, index, 0, 4);
584+
585+
// If size > 0 we should cache if this is enabled via cluster setting
586+
ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest();
587+
updateSettingsRequest.persistentSettings(
588+
Settings.builder().put(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.getKey(), true)
589+
);
590+
assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
591+
592+
final SearchResponse r7 = client.prepareSearch(index)
593+
.setSearchType(SearchType.QUERY_THEN_FETCH)
594+
.setSize(1)
595+
.setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-22").lte("2016-03-26"))
596+
.get();
597+
OpenSearchAssertions.assertAllSuccessful(r7);
598+
assertThat(r7.getHits().getTotalHits().value, equalTo(5L));
599+
assertCacheState(client, index, 0, 6);
582600
}
583601

584602
public void testCacheWithFilteredAlias() throws InterruptedException {

‎server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

+1
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,7 @@ public void apply(Settings value, Settings current, Settings previous) {
519519
IndicesRequestCache.INDICES_CACHE_QUERY_EXPIRE,
520520
IndicesRequestCache.INDICES_REQUEST_CACHE_CLEANUP_INTERVAL_SETTING,
521521
IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING,
522+
IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING,
522523
HunspellService.HUNSPELL_LAZY_LOAD,
523524
HunspellService.HUNSPELL_IGNORE_CASE,
524525
HunspellService.HUNSPELL_DICTIONARY_OPTIONS,

‎server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

+12
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,18 @@ public final class IndicesRequestCache implements RemovalListener<ICacheKey<Indi
147147
Property.NodeScope
148148
);
149149

150+
/**
151+
* If enabled, allows caching all cacheable queries. For now, this means also allowing size > 0 queries.
152+
* If enabled, fundamentally non-cacheable queries like DFS queries, queries using the `now` keyword, and
153+
* scroll requests are still not cached.
154+
*/
155+
public static final Setting<Boolean> INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING = Setting.boolSetting(
156+
"indices.requests.cache.enable_for_all_requests",
157+
false,
158+
Property.NodeScope,
159+
Property.Dynamic
160+
);
161+
150162
private final static long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(Key.class);
151163

152164
private final ConcurrentMap<CleanupKey, Boolean> registeredClosedListeners = ConcurrentCollections.newConcurrentMap();

‎server/src/main/java/org/opensearch/indices/IndicesService.java

+13-3
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@
205205
import static org.opensearch.index.IndexService.IndexCreationContext.CREATE_INDEX;
206206
import static org.opensearch.index.IndexService.IndexCreationContext.METADATA_VERIFICATION;
207207
import static org.opensearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
208+
import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING;
208209
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.isRemoteDataAttributePresent;
209210
import static org.opensearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES;
210211

@@ -360,6 +361,7 @@ public class IndicesService extends AbstractLifecycleComponent
360361
private final FileCache fileCache;
361362
private final CompositeIndexSettings compositeIndexSettings;
362363
private final Consumer<IndexShard> replicator;
364+
private volatile boolean requestCachingEnabledForAllQueries;
363365

364366
@Override
365367
protected void doStart() {
@@ -507,6 +509,9 @@ protected void closeInternal() {
507509
this.compositeIndexSettings = compositeIndexSettings;
508510
this.fileCache = fileCache;
509511
this.replicator = replicator;
512+
this.requestCachingEnabledForAllQueries = INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING.get(clusterService.getSettings());
513+
clusterService.getClusterSettings()
514+
.addSettingsUpdateConsumer(INDICES_REQUEST_CACHE_ENABLE_FOR_ALL_REQUESTS_SETTING, this::setRequestCachingEnabledForAllQueries);
510515
}
511516

512517
public IndicesService(
@@ -1746,11 +1751,11 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) {
17461751
IndexSettings settings = context.indexShard().indexSettings();
17471752
// if not explicitly set in the request, use the index setting, if not, use the request
17481753
if (request.requestCache() == null) {
1749-
if (settings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING) == false) {
1750-
return false;
1751-
} else if (context.size() != 0) {
1754+
if (settings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING) == false
1755+
|| (context.size() > 0 && !requestCachingEnabledForAllQueries)) {
17521756
// If no request cache query parameter and shard request cache
17531757
// is enabled in settings don't cache for requests with size > 0
1758+
// unless this is enabled via cluster setting
17541759
return false;
17551760
}
17561761
} else if (request.requestCache() == false) {
@@ -2118,4 +2123,9 @@ public RemoteStoreSettings getRemoteStoreSettings() {
21182123
public CompositeIndexSettings getCompositeIndexSettings() {
21192124
return this.compositeIndexSettings;
21202125
}
2126+
2127+
// Package-private for testing
2128+
void setRequestCachingEnabledForAllQueries(Boolean requestCachingEnabledForAllQueries) {
2129+
this.requestCachingEnabledForAllQueries = requestCachingEnabledForAllQueries;
2130+
}
21212131
}

‎server/src/test/java/org/opensearch/indices/IndicesServiceTests.java

+56-13
Original file line numberDiff line numberDiff line change
@@ -641,25 +641,68 @@ public void testDirectoryReaderWithoutDelegatingCacheHelperNotCacheable() throws
641641
ShardSearchRequest request = mock(ShardSearchRequest.class);
642642
when(request.requestCache()).thenReturn(true);
643643

644-
TestSearchContext context = new TestSearchContext(indexService.getBigArrays(), indexService) {
645-
@Override
646-
public SearchType searchType() {
647-
return SearchType.QUERY_THEN_FETCH;
648-
}
649-
};
644+
TestSearchContext context = getTestContext(indexService, 0);
645+
IndexReader.CacheHelper notDelegatingCacheHelper = mock(IndexReader.CacheHelper.class);
646+
DelegatingCacheHelper delegatingCacheHelper = mock(DelegatingCacheHelper.class);
647+
for (boolean useDelegatingCacheHelper : new boolean[] { true, false }) {
648+
IndexReader.CacheHelper cacheHelper = useDelegatingCacheHelper ? delegatingCacheHelper : notDelegatingCacheHelper;
649+
setupMocksForCanCache(context, cacheHelper);
650+
assertEquals(useDelegatingCacheHelper, indicesService.canCache(request, context));
651+
}
652+
}
653+
654+
public void testCanCacheSizeNonzero() {
655+
// Size == 0 requests should always be cacheable (if they pass the other checks).
656+
// Size > 0 requests should only be cacheable if ALLOW_SIZE_NONZERO_SETTING is true.
657+
658+
final IndexService indexService = createIndex("test");
659+
ShardSearchRequest request = mock(ShardSearchRequest.class);
660+
when(request.requestCache()).thenReturn(null);
661+
662+
TestSearchContext sizeZeroContext = getTestContext(indexService, 0);
663+
TestSearchContext sizeNonzeroContext = getTestContext(indexService, 10);
664+
665+
// Test for an IndicesService with the default setting value of false
666+
IndicesService indicesService = getIndicesService();
667+
DelegatingCacheHelper cacheHelper = mock(DelegatingCacheHelper.class);
668+
Map<TestSearchContext, Boolean> expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, false);
650669

670+
for (Map.Entry<TestSearchContext, Boolean> entry : expectedResultMap.entrySet()) {
671+
TestSearchContext context = entry.getKey();
672+
setupMocksForCanCache(context, cacheHelper);
673+
assertEquals(entry.getValue(), indicesService.canCache(request, context));
674+
}
675+
// Simulate the cluster setting update by manually calling setCanCacheSizeNonzeroRequests
676+
indicesService.setRequestCachingEnabledForAllQueries(true);
677+
expectedResultMap = Map.of(sizeZeroContext, true, sizeNonzeroContext, true);
678+
679+
for (Map.Entry<TestSearchContext, Boolean> entry : expectedResultMap.entrySet()) {
680+
TestSearchContext context = entry.getKey();
681+
setupMocksForCanCache(context, cacheHelper);
682+
assertEquals(entry.getValue(), indicesService.canCache(request, context));
683+
}
684+
}
685+
686+
private void setupMocksForCanCache(TestSearchContext context, IndexReader.CacheHelper cacheHelper) {
651687
ContextIndexSearcher searcher = mock(ContextIndexSearcher.class);
652688
context.setSearcher(searcher);
653689
DirectoryReader reader = mock(DirectoryReader.class);
654690
when(searcher.getDirectoryReader()).thenReturn(reader);
655691
when(searcher.getIndexReader()).thenReturn(reader);
656-
IndexReader.CacheHelper notDelegatingCacheHelper = mock(IndexReader.CacheHelper.class);
657-
DelegatingCacheHelper delegatingCacheHelper = mock(DelegatingCacheHelper.class);
692+
when(reader.getReaderCacheHelper()).thenReturn(cacheHelper);
693+
}
658694

659-
for (boolean useDelegatingCacheHelper : new boolean[] { true, false }) {
660-
IndexReader.CacheHelper cacheHelper = useDelegatingCacheHelper ? delegatingCacheHelper : notDelegatingCacheHelper;
661-
when(reader.getReaderCacheHelper()).thenReturn(cacheHelper);
662-
assertEquals(useDelegatingCacheHelper, indicesService.canCache(request, context));
663-
}
695+
private TestSearchContext getTestContext(IndexService indexService, int size) {
696+
return new TestSearchContext(indexService.getBigArrays(), indexService) {
697+
@Override
698+
public SearchType searchType() {
699+
return SearchType.QUERY_THEN_FETCH;
700+
}
701+
702+
@Override
703+
public int size() {
704+
return size;
705+
}
706+
};
664707
}
665708
}

0 commit comments

Comments
 (0)