From c9dff27f2050210572eda80d08a773a7791efad1 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 01:45:54 -0700 Subject: [PATCH 1/7] Adding cache eviction and listener for invalidating index field type mappings on index deletion/update Signed-off-by: Ankit Jain --- .../categorizer/IndicesFieldTypeCache.java | 78 +++++++++++++++++++ .../categorizer/QueryShapeGenerator.java | 35 +++++++-- .../categorizer/QueryShapeGeneratorTests.java | 1 + 3 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java new file mode 100644 index 0000000..5096eeb --- /dev/null +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java @@ -0,0 +1,78 @@ +package org.opensearch.plugin.insights.core.service.categorizer; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.lucene.util.RamUsageEstimator; +import org.opensearch.common.cache.Cache; +import org.opensearch.common.cache.CacheBuilder; +import org.opensearch.common.metrics.CounterMetric; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; +import org.opensearch.core.common.unit.ByteSizeValue; +import org.opensearch.core.index.Index; +import org.opensearch.indices.fielddata.cache.IndicesFieldDataCache; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; + +public class IndicesFieldTypeCache { + + private static final Logger logger = LogManager.getLogger(IndicesFieldTypeCache.class); + public static final Setting INDICES_FIELD_TYPE_CACHE_SIZE_KEY = Setting.memorySizeSetting( + "search.insights.indices.fieldtype.cache.size", + new ByteSizeValue(-1), + Setting.Property.NodeScope + ); + private final Cache cache; + + public IndicesFieldTypeCache(Settings settings) { + final long sizeInBytes = -1; //TODO: INDICES_FIELD_TYPE_CACHE_SIZE_KEY.get(settings).getBytes(); + CacheBuilder cacheBuilder = CacheBuilder.builder(); + if (sizeInBytes > 0) { + cacheBuilder.setMaximumWeight(sizeInBytes).weigher((k, v) -> RamUsageEstimator.sizeOfObject(k) + v.weight()); + } + cache = cacheBuilder.build(); + } + + public IndexFieldMap getOrInitialize(Index index) { + try { + return cache.computeIfAbsent(index, k -> new IndexFieldMap()); + } catch (ExecutionException ex) { + logger.error("Unexpected execution exception while initializing for index " + index); + } + + return null; + } + + public void invalidate(Index index) { + cache.invalidate(index); + } + + public Iterable keySet() { + return cache.keys(); + } + + static class IndexFieldMap { + private ConcurrentHashMap fieldTypeMap; + private CounterMetric weight; + IndexFieldMap() { + fieldTypeMap = new ConcurrentHashMap<>(); + weight = new CounterMetric(); + } + + public String get(String fieldName) { + return fieldTypeMap.get(fieldName); + } + + public void putIfAbsent(String key, String value) { + // Increment the weight only if the key value got added to the Map + if (fieldTypeMap.putIfAbsent(key, value) == null) { + weight.inc(RamUsageEstimator.sizeOf(key) + RamUsageEstimator.sizeOf(value)); + } + } + public long weight() { + return weight.count(); + } + } +} + diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java index f16257b..aa66535 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java @@ -16,8 +16,13 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; + import org.apache.lucene.util.BytesRef; +import org.opensearch.cluster.ClusterChangedEvent; +import org.opensearch.cluster.ClusterStateListener; import org.opensearch.cluster.metadata.MappingMetadata; +import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.hash.MurmurHash3; import org.opensearch.core.common.io.stream.NamedWriteable; @@ -33,16 +38,36 @@ /** * Class to generate query shape */ -public class QueryShapeGenerator { +public class QueryShapeGenerator implements ClusterStateListener { static final String EMPTY_STRING = ""; static final String ONE_SPACE_INDENT = " "; private final ClusterService clusterService; private final String NO_FIELD_TYPE_VALUE = ""; - private final ConcurrentHashMap> fieldTypeMap; + private final IndicesFieldTypeCache indicesFieldTypeCache; public QueryShapeGenerator(ClusterService clusterService) { this.clusterService = clusterService; - this.fieldTypeMap = new ConcurrentHashMap<>(); + clusterService.addListener(this); + this.indicesFieldTypeCache = new IndicesFieldTypeCache(clusterService.getSettings()); + } + + public void clusterChanged(ClusterChangedEvent event) { + final List indicesDeleted = event.indicesDeleted(); + for (Index index : indicesDeleted) { + // remove the deleted index mapping from field type cache + indicesFieldTypeCache.invalidate(index); + } + + if (event.metadataChanged()) { + final Metadata previousMetadata = event.previousState().metadata(); + final Metadata currentMetadata = event.state().metadata(); + for (Index index : indicesFieldTypeCache.keySet()) { + if (previousMetadata.index(index) != currentMetadata.index(index)) { + // remove the updated index mapping from field type cache + indicesFieldTypeCache.invalidate(index); + } + } + } } /** @@ -363,7 +388,7 @@ String getFieldType(String fieldName, Map propertiesAsMap, Index fieldType = getFieldTypeFromProperties(fieldName, propertiesAsMap); // Cache field type or NO_FIELD_TYPE_VALUE if not found - fieldTypeMap.computeIfAbsent(index, k -> new ConcurrentHashMap<>()) + indicesFieldTypeCache.getOrInitialize(index) .putIfAbsent(fieldName, fieldType != null ? fieldType : NO_FIELD_TYPE_VALUE); return fieldType; @@ -406,6 +431,6 @@ else if (currentMap.containsKey("type")) { } String getFieldTypeFromCache(String fieldName, Index index) { - return fieldTypeMap.getOrDefault(index, new ConcurrentHashMap<>()).get(fieldName); + return indicesFieldTypeCache.getOrInitialize(index).get(fieldName); } } diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java index dc603a8..ae0af31 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java @@ -36,6 +36,7 @@ import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.hash.MurmurHash3; +import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.DistanceUnit; import org.opensearch.core.compress.CompressorRegistry; import org.opensearch.core.index.Index; From 3908dbd1d8f7c387fbd1c5fc63cb9510d6ef118d Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 10:00:31 -0700 Subject: [PATCH 2/7] Fixing spotless violations Signed-off-by: Ankit Jain --- .../service/categorizer/IndicesFieldTypeCache.java | 11 +++++------ .../core/service/categorizer/QueryShapeGenerator.java | 6 +----- .../service/categorizer/QueryShapeGeneratorTests.java | 1 - 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java index 5096eeb..0816a3b 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java @@ -1,5 +1,7 @@ package org.opensearch.plugin.insights.core.service.categorizer; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.util.RamUsageEstimator; @@ -10,10 +12,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.index.Index; -import org.opensearch.indices.fielddata.cache.IndicesFieldDataCache; - -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ExecutionException; public class IndicesFieldTypeCache { @@ -26,7 +24,7 @@ public class IndicesFieldTypeCache { private final Cache cache; public IndicesFieldTypeCache(Settings settings) { - final long sizeInBytes = -1; //TODO: INDICES_FIELD_TYPE_CACHE_SIZE_KEY.get(settings).getBytes(); + final long sizeInBytes = -1; // TODO: INDICES_FIELD_TYPE_CACHE_SIZE_KEY.get(settings).getBytes(); CacheBuilder cacheBuilder = CacheBuilder.builder(); if (sizeInBytes > 0) { cacheBuilder.setMaximumWeight(sizeInBytes).weigher((k, v) -> RamUsageEstimator.sizeOfObject(k) + v.weight()); @@ -55,6 +53,7 @@ public Iterable keySet() { static class IndexFieldMap { private ConcurrentHashMap fieldTypeMap; private CounterMetric weight; + IndexFieldMap() { fieldTypeMap = new ConcurrentHashMap<>(); weight = new CounterMetric(); @@ -70,9 +69,9 @@ public void putIfAbsent(String key, String value) { weight.inc(RamUsageEstimator.sizeOf(key) + RamUsageEstimator.sizeOf(value)); } } + public long weight() { return weight.count(); } } } - diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java index aa66535..a02259a 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java @@ -15,9 +15,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ExecutionException; - import org.apache.lucene.util.BytesRef; import org.opensearch.cluster.ClusterChangedEvent; import org.opensearch.cluster.ClusterStateListener; @@ -388,8 +385,7 @@ String getFieldType(String fieldName, Map propertiesAsMap, Index fieldType = getFieldTypeFromProperties(fieldName, propertiesAsMap); // Cache field type or NO_FIELD_TYPE_VALUE if not found - indicesFieldTypeCache.getOrInitialize(index) - .putIfAbsent(fieldName, fieldType != null ? fieldType : NO_FIELD_TYPE_VALUE); + indicesFieldTypeCache.getOrInitialize(index).putIfAbsent(fieldName, fieldType != null ? fieldType : NO_FIELD_TYPE_VALUE); return fieldType; } diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java index ae0af31..dc603a8 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java @@ -36,7 +36,6 @@ import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.hash.MurmurHash3; -import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.DistanceUnit; import org.opensearch.core.compress.CompressorRegistry; import org.opensearch.core.index.Index; From 482db81658013b3b299bc7ef3badc8d3ceb3ded6 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 10:21:41 -0700 Subject: [PATCH 3/7] Fixing code to use .index instead of .findMappings Signed-off-by: Ankit Jain --- .../plugin/insights/QueryInsightsPlugin.java | 4 +- .../categorizer/QueryShapeGenerator.java | 17 ++--- .../categorizer/QueryShapeGeneratorTests.java | 62 ++++++++++--------- 3 files changed, 39 insertions(+), 44 deletions(-) diff --git a/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java b/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java index 862f4c8..3c78afd 100644 --- a/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java +++ b/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java @@ -31,6 +31,7 @@ import org.opensearch.plugin.insights.core.listener.QueryInsightsListener; import org.opensearch.plugin.insights.core.metrics.OperationalMetricsCounter; import org.opensearch.plugin.insights.core.service.QueryInsightsService; +import org.opensearch.plugin.insights.core.service.categorizer.IndicesFieldTypeCache; import org.opensearch.plugin.insights.rules.action.health_stats.HealthStatsAction; import org.opensearch.plugin.insights.rules.action.top_queries.TopQueriesAction; import org.opensearch.plugin.insights.rules.resthandler.health_stats.RestHealthStatsAction; @@ -144,7 +145,8 @@ public List> getSettings() { QueryInsightsSettings.TOP_N_QUERIES_MAX_GROUPS_EXCLUDING_N, QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_NAME, QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_TYPE, - QueryCategorizationSettings.SEARCH_QUERY_METRICS_ENABLED_SETTING + QueryCategorizationSettings.SEARCH_QUERY_METRICS_ENABLED_SETTING, + IndicesFieldTypeCache.INDICES_FIELD_TYPE_CACHE_SIZE_KEY ); } } diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java index a02259a..191c606 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java @@ -8,7 +8,6 @@ package org.opensearch.plugin.insights.core.service.categorizer; -import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -18,7 +17,7 @@ import org.apache.lucene.util.BytesRef; import org.opensearch.cluster.ClusterChangedEvent; import org.opensearch.cluster.ClusterStateListener; -import org.opensearch.cluster.metadata.MappingMetadata; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.hash.MurmurHash3; @@ -149,20 +148,12 @@ public String buildShape( } private Map getPropertiesMapForIndex(Index index) { - Map indexMapping; - try { - indexMapping = clusterService.state().metadata().findMappings(new String[] { index.getName() }, input -> str -> true); - } catch (IOException e) { - // If an error occurs while retrieving mappings, return an empty map + IndexMetadata indexMetadata = clusterService.state().metadata().index(index); + if (indexMetadata == null) { return Collections.emptyMap(); } - MappingMetadata mappingMetadata = indexMapping.get(index.getName()); - if (mappingMetadata == null) { - return Collections.emptyMap(); - } - - Map propertiesMap = (Map) mappingMetadata.getSourceAsMap().get("properties"); + Map propertiesMap = (Map) indexMetadata.mapping().getSourceAsMap().get("properties"); if (propertiesMap == null) { return Collections.emptyMap(); } diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java index dc603a8..19ca1d3 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java @@ -32,6 +32,7 @@ import java.util.UUID; import org.apache.lucene.search.join.ScoreMode; import org.opensearch.cluster.ClusterState; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; @@ -52,7 +53,9 @@ import org.opensearch.test.OpenSearchTestCase; public final class QueryShapeGeneratorTests extends OpenSearchTestCase { - final Set successfulSearchShardIndices = Set.of(new Index("index1", UUID.randomUUID().toString())); + final Index index1 = new Index("index1", UUID.randomUUID().toString()); + final Index index2 = new Index("index2", UUID.randomUUID().toString()); + final Set successfulSearchShardIndices = Set.of(index1); final QueryShapeGenerator queryShapeGenerator; private ClusterService mockClusterService; @@ -70,18 +73,17 @@ public QueryShapeGeneratorTests() { when(mockClusterState.metadata()).thenReturn(mockMetaData); } - public void setUpMockMappings(String indexName, Map mappingProperties) throws IOException { + public void setUpMockMappings(Index index, Map mappingProperties) throws IOException { MappingMetadata mockMappingMetadata = mock(MappingMetadata.class); - when(mockMappingMetadata.getSourceAsMap()).thenReturn(mappingProperties); - - final Map indexMappingMap = Map.of(indexName, mockMappingMetadata); - when(mockMetaData.findMappings(any(), any())).thenReturn(indexMappingMap); + IndexMetadata indexMetadata = mock(IndexMetadata.class); + when(indexMetadata.mapping()).thenReturn(mockMappingMetadata); + when(mockMetaData.index(index)).thenReturn(indexMetadata); } public void testBasicSearchWithFieldNameAndType() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -113,7 +115,7 @@ public void testBasicSearchWithFieldNameAndType() throws IOException { // If field type is not found we leave the field type blank public void testFieldTypeNotFound() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); + setUpMockMappings(index1, Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query( @@ -139,7 +141,7 @@ public void testFieldTypeNotFound() throws IOException { public void testEmptyMappings() throws IOException { setUpMockMappings( - "index1", + successfulSearchShardIndices.iterator().next(), Map.of( "properties", Map.of() // No fields defined @@ -170,9 +172,9 @@ public void testEmptyMappings() throws IOException { // Field type should be inferred from both the mappings public void testMultipleIndexMappings() throws IOException { - setUpMockMappings("index2", Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); + setUpMockMappings(index2, Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); - setUpMockMappings("index1", Map.of("properties", Map.of("field2", Map.of("type", "text"), "field4", Map.of("type", "long")))); + setUpMockMappings(index1, Map.of("properties", Map.of("field2", Map.of("type", "text"), "field4", Map.of("type", "long")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder(); @@ -190,7 +192,7 @@ public void testMultipleIndexMappings() throws IOException { public void testDifferentFieldTypes() throws IOException { setUpMockMappings( - "index1", + successfulSearchShardIndices.iterator().next(), Map.of( "properties", Map.of( @@ -222,7 +224,7 @@ public void testDifferentFieldTypes() throws IOException { public void testFieldWithNestedProperties() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -251,7 +253,7 @@ public void testFieldWithNestedProperties() throws IOException { public void testFieldWithArrayType() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of("field1", Map.of("type", "keyword"), "field2", Map.of("type", "text"), "field3", Map.of("type", "keyword")) @@ -277,7 +279,7 @@ public void testFieldWithArrayType() throws IOException { } public void testFieldWithDateType() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("dateField", Map.of("type", "date")))); + setUpMockMappings(index1, Map.of("properties", Map.of("dateField", Map.of("type", "date")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(boolQuery().filter(rangeQuery("dateField").gte("2024-01-01").lte("2024-12-31"))); @@ -288,7 +290,7 @@ public void testFieldWithDateType() throws IOException { } public void testFieldWithGeoPointType() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("location", Map.of("type", "geo_point")))); + setUpMockMappings(index1, Map.of("properties", Map.of("location", Map.of("type", "geo_point")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(boolQuery().filter(geoDistanceQuery("location").point(40.73, -74.1).distance(200, DistanceUnit.KILOMETERS))); @@ -299,7 +301,7 @@ public void testFieldWithGeoPointType() throws IOException { } public void testFieldWithBinaryType() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("binaryField", Map.of("type", "binary")))); + setUpMockMappings(successfulSearchShardIndices.iterator().next(), Map.of("properties", Map.of("binaryField", Map.of("type", "binary")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(boolQuery().must(termQuery("binaryField", "base64EncodedString"))); @@ -311,7 +313,7 @@ public void testFieldWithBinaryType() throws IOException { public void testFieldWithMixedTypes() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -339,7 +341,7 @@ public void testFieldWithMixedTypes() throws IOException { } public void testFieldWithInvalidQueries() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); + setUpMockMappings(index1, Map.of("properties", Map.of("field1", Map.of("type", "keyword")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query( @@ -353,7 +355,7 @@ public void testFieldWithInvalidQueries() throws IOException { public void testFieldWithDeeplyNestedStructure() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -378,7 +380,7 @@ public void testFieldWithDeeplyNestedStructure() throws IOException { // We are not parsing fields for scripts public void testFieldWithScriptedQuery() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("scriptedField", Map.of("type", "long")))); + setUpMockMappings(index1, Map.of("properties", Map.of("scriptedField", Map.of("type", "long")))); Script script = new Script( ScriptType.INLINE, @@ -398,7 +400,7 @@ public void testFieldWithScriptedQuery() throws IOException { public void testDynamicTemplateMappingWithTypeInference() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "dynamic_templates", List.of( @@ -424,7 +426,7 @@ public void testDynamicTemplateMappingWithTypeInference() throws IOException { } public void testFieldWithIpAddressType() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("ip_address", Map.of("type", "ip", "ignore_malformed", true)))); + setUpMockMappings(index1, Map.of("properties", Map.of("ip_address", Map.of("type", "ip", "ignore_malformed", true)))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(boolQuery().must(termQuery("ip_address", "192.168.1.1")).filter(termQuery("ip_address", "invalid_ip"))); @@ -442,7 +444,7 @@ public void testFieldWithIpAddressType() throws IOException { // Nested query not working as expected public void testNestedQueryType() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -475,7 +477,7 @@ public void testNestedQueryType() throws IOException { public void testFlatObjectQueryType() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of( @@ -515,7 +517,7 @@ public void testFlatObjectQueryType() throws IOException { public void testQueryTypeWithSorting() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of("age", Map.of("type", "integer"), "name", Map.of("type", "keyword"), "score", Map.of("type", "float")) @@ -535,7 +537,7 @@ public void testQueryTypeWithSorting() throws IOException { } public void testQueryTypeWithAggregations() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("price", Map.of("type", "double"), "category", Map.of("type", "keyword")))); + setUpMockMappings(index1, Map.of("properties", Map.of("price", Map.of("type", "double"), "category", Map.of("type", "keyword")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(matchAllQuery()) @@ -551,7 +553,7 @@ public void testQueryTypeWithAggregations() throws IOException { // No field name and type being parsed for pipeline aggregations public void testQueryTypeWithPipelineAggregation() throws IOException { - setUpMockMappings("index1", Map.of("properties", Map.of("sales", Map.of("type", "double"), "timestamp", Map.of("type", "date")))); + setUpMockMappings(index1, Map.of("properties", Map.of("sales", Map.of("type", "double"), "timestamp", Map.of("type", "date")))); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(matchAllQuery()) @@ -579,7 +581,7 @@ public void testQueryTypeWithPipelineAggregation() throws IOException { // Should cache empty value when we do not find a field type to avoid doing the search again public void testFieldTypeCachingForNonExistentField() throws IOException { setUpMockMappings( - "index1", + index1, Map.of( "properties", Map.of("age", Map.of("type", "integer"), "name", Map.of("type", "keyword"), "score", Map.of("type", "float")) @@ -603,7 +605,7 @@ public void testFieldTypeCachingForNonExistentField() throws IOException { public void testMultifieldQueryCombined() throws IOException { setUpMockMappings( - "index1", + index1, Map.of("properties", Map.of("title", Map.of("type", "text", "fields", Map.of("raw", Map.of("type", "keyword"))))) ); From da2e3dd726c0cf64a79659ae72e9a10b3acb83c1 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 10:23:51 -0700 Subject: [PATCH 4/7] Fixing spotless violations Signed-off-by: Ankit Jain --- .../core/service/categorizer/QueryShapeGeneratorTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java index 19ca1d3..f6b8f2a 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java @@ -301,7 +301,10 @@ public void testFieldWithGeoPointType() throws IOException { } public void testFieldWithBinaryType() throws IOException { - setUpMockMappings(successfulSearchShardIndices.iterator().next(), Map.of("properties", Map.of("binaryField", Map.of("type", "binary")))); + setUpMockMappings( + successfulSearchShardIndices.iterator().next(), + Map.of("properties", Map.of("binaryField", Map.of("type", "binary"))) + ); SearchSourceBuilder sourceBuilder = SearchSourceBuilderUtils.createQuerySearchSourceBuilder() .query(boolQuery().must(termQuery("binaryField", "base64EncodedString"))); From d55a3d2660957ecede9977fd86c3132713105351 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 12:35:03 -0700 Subject: [PATCH 5/7] Addressing review comments Signed-off-by: Ankit Jain --- .../plugin/insights/QueryInsightsPlugin.java | 3 +-- .../categorizer/IndicesFieldTypeCache.java | 27 ++++++++++++------- .../settings/QueryCategorizationSettings.java | 7 +++++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java b/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java index 3c78afd..26d86e9 100644 --- a/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java +++ b/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java @@ -31,7 +31,6 @@ import org.opensearch.plugin.insights.core.listener.QueryInsightsListener; import org.opensearch.plugin.insights.core.metrics.OperationalMetricsCounter; import org.opensearch.plugin.insights.core.service.QueryInsightsService; -import org.opensearch.plugin.insights.core.service.categorizer.IndicesFieldTypeCache; import org.opensearch.plugin.insights.rules.action.health_stats.HealthStatsAction; import org.opensearch.plugin.insights.rules.action.top_queries.TopQueriesAction; import org.opensearch.plugin.insights.rules.resthandler.health_stats.RestHealthStatsAction; @@ -146,7 +145,7 @@ public List> getSettings() { QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_NAME, QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_TYPE, QueryCategorizationSettings.SEARCH_QUERY_METRICS_ENABLED_SETTING, - IndicesFieldTypeCache.INDICES_FIELD_TYPE_CACHE_SIZE_KEY + QueryCategorizationSettings.SEARCH_QUERY_FIELD_TYPE_CACHE_SIZE_KEY ); } } diff --git a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java index 0816a3b..abf7975 100644 --- a/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java +++ b/src/main/java/org/opensearch/plugin/insights/core/service/categorizer/IndicesFieldTypeCache.java @@ -1,3 +1,11 @@ +/* + * 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.plugin.insights.core.service.categorizer; import java.util.concurrent.ConcurrentHashMap; @@ -8,23 +16,21 @@ import org.opensearch.common.cache.Cache; import org.opensearch.common.cache.CacheBuilder; import org.opensearch.common.metrics.CounterMetric; -import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; -import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.index.Index; +import org.opensearch.plugin.insights.settings.QueryCategorizationSettings; +/** + * Cache implementation specifically for maintaining the field name type mappings + * for indices that are part of successful search requests + */ public class IndicesFieldTypeCache { private static final Logger logger = LogManager.getLogger(IndicesFieldTypeCache.class); - public static final Setting INDICES_FIELD_TYPE_CACHE_SIZE_KEY = Setting.memorySizeSetting( - "search.insights.indices.fieldtype.cache.size", - new ByteSizeValue(-1), - Setting.Property.NodeScope - ); private final Cache cache; public IndicesFieldTypeCache(Settings settings) { - final long sizeInBytes = -1; // TODO: INDICES_FIELD_TYPE_CACHE_SIZE_KEY.get(settings).getBytes(); + final long sizeInBytes = QueryCategorizationSettings.SEARCH_QUERY_FIELD_TYPE_CACHE_SIZE_KEY.get(settings).getBytes(); CacheBuilder cacheBuilder = CacheBuilder.builder(); if (sizeInBytes > 0) { cacheBuilder.setMaximumWeight(sizeInBytes).weigher((k, v) -> RamUsageEstimator.sizeOfObject(k) + v.weight()); @@ -39,6 +45,9 @@ public IndexFieldMap getOrInitialize(Index index) { logger.error("Unexpected execution exception while initializing for index " + index); } + // Should never return null as the ExecutionException is only thrown + // if loader throws an exception or returns a null value, which cannot + // be the case in this scenario return null; } @@ -64,7 +73,7 @@ public String get(String fieldName) { } public void putIfAbsent(String key, String value) { - // Increment the weight only if the key value got added to the Map + // Increment the weight only if the key value pair added to the Map if (fieldTypeMap.putIfAbsent(key, value) == null) { weight.inc(RamUsageEstimator.sizeOf(key) + RamUsageEstimator.sizeOf(value)); } diff --git a/src/main/java/org/opensearch/plugin/insights/settings/QueryCategorizationSettings.java b/src/main/java/org/opensearch/plugin/insights/settings/QueryCategorizationSettings.java index a5a65af..ec9c6af 100644 --- a/src/main/java/org/opensearch/plugin/insights/settings/QueryCategorizationSettings.java +++ b/src/main/java/org/opensearch/plugin/insights/settings/QueryCategorizationSettings.java @@ -9,6 +9,7 @@ package org.opensearch.plugin.insights.settings; import org.opensearch.common.settings.Setting; +import org.opensearch.core.common.unit.ByteSizeValue; /** * Settings for Query Categorization @@ -24,6 +25,12 @@ public class QueryCategorizationSettings { Setting.Property.Dynamic ); + public static final Setting SEARCH_QUERY_FIELD_TYPE_CACHE_SIZE_KEY = Setting.memorySizeSetting( + "search.query.fieldtype.cache.size", + "0.1%", + Setting.Property.NodeScope + ); + /** * Default constructor */ From 6464cdae1854db2116ab2d5210e53a633163e66d Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 12:43:05 -0700 Subject: [PATCH 6/7] Fixing existing test failures Signed-off-by: Ankit Jain --- .../core/service/categorizer/QueryShapeGeneratorTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java index f6b8f2a..f3eb25f 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGeneratorTests.java @@ -37,6 +37,7 @@ import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.hash.MurmurHash3; +import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.DistanceUnit; import org.opensearch.core.compress.CompressorRegistry; import org.opensearch.core.index.Index; @@ -65,6 +66,7 @@ public final class QueryShapeGeneratorTests extends OpenSearchTestCase { public QueryShapeGeneratorTests() { CompressorRegistry.defaultCompressor(); this.mockClusterService = mock(ClusterService.class); + when(mockClusterService.getSettings()).thenReturn(Settings.EMPTY); this.mockClusterState = mock(ClusterState.class); this.mockMetaData = mock(Metadata.class); this.queryShapeGenerator = new QueryShapeGenerator(mockClusterService); From 010bafb9fd9878a0ea46276c38ce940942723e0c Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 10 Oct 2024 12:50:15 -0700 Subject: [PATCH 7/7] Fixing existing test failures Signed-off-by: Ankit Jain --- .../plugin/insights/QueryInsightsPluginTests.java | 3 ++- .../core/service/categorizer/QueryShapeVisitorTests.java | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/opensearch/plugin/insights/QueryInsightsPluginTests.java b/src/test/java/org/opensearch/plugin/insights/QueryInsightsPluginTests.java index e695994..de4354b 100644 --- a/src/test/java/org/opensearch/plugin/insights/QueryInsightsPluginTests.java +++ b/src/test/java/org/opensearch/plugin/insights/QueryInsightsPluginTests.java @@ -81,7 +81,8 @@ public void testGetSettings() { QueryInsightsSettings.TOP_N_QUERIES_MAX_GROUPS_EXCLUDING_N, QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_NAME, QueryInsightsSettings.TOP_N_QUERIES_GROUPING_FIELD_TYPE, - QueryCategorizationSettings.SEARCH_QUERY_METRICS_ENABLED_SETTING + QueryCategorizationSettings.SEARCH_QUERY_METRICS_ENABLED_SETTING, + QueryCategorizationSettings.SEARCH_QUERY_FIELD_TYPE_CACHE_SIZE_KEY ), queryInsightsPlugin.getSettings() ); diff --git a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeVisitorTests.java b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeVisitorTests.java index 5c9e4ba..eb363e4 100644 --- a/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeVisitorTests.java +++ b/src/test/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeVisitorTests.java @@ -9,9 +9,11 @@ package org.opensearch.plugin.insights.core.service.categorizer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.util.HashMap; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.ConstantScoreQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; @@ -31,8 +33,10 @@ public void testQueryShapeVisitor() { .mustNot(new RegexpQueryBuilder("color", "red.*")) ) .must(new TermsQueryBuilder("genre", "action", "drama", "romance")); + final ClusterService mockClusterService = mock(ClusterService.class); + when(mockClusterService.getSettings()).thenReturn(Settings.EMPTY); QueryShapeVisitor shapeVisitor = new QueryShapeVisitor( - new QueryShapeGenerator(mock(ClusterService.class)), + new QueryShapeGenerator(mockClusterService), new HashMap<>(), null, false,