From fd15f47acb3b568adad315f96c38c671135d8030 Mon Sep 17 00:00:00 2001 From: Sorabh Hamirwasia Date: Tue, 25 Jul 2023 10:09:16 -0700 Subject: [PATCH] Address review comments --- .../search/SearchBootstrapSettings.java | 12 +++++++-- .../search/internal/ContextIndexSearcher.java | 16 +++++------- .../internal/MaxTargetSliceSupplier.java | 26 +++++++------------ .../test/OpenSearchSingleNodeTestCase.java | 9 ++++--- 4 files changed, 32 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java b/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java index 711c4b9b15acd..c8903ec6a44b6 100644 --- a/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java +++ b/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java @@ -13,6 +13,8 @@ /** * Keeps track of all the search related node level settings which can be accessed via static methods + * + * @opensearch.internal */ public class SearchBootstrapSettings { // settings to configure maximum slice created per search request using OS custom slice computation mechanism. Default lucene @@ -21,6 +23,7 @@ public class SearchBootstrapSettings { public static final int CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE = -1; // value <= 0 means lucene slice computation will be used + // this setting will be updated to dynamic setting as part of https://github.com/opensearch-project/OpenSearch/issues/8870 public static final Setting CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING = Setting.intSetting( CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE, @@ -32,7 +35,12 @@ public static void initialize(Settings openSearchSettings) { settings = openSearchSettings; } - public static int getValueAsInt(String settingName, int defaultValue) { - return (settings != null) ? settings.getAsInt(settingName, defaultValue) : defaultValue; + public static int getTargetMaxSlice() { + return (settings != null) + ? settings.getAsInt( + CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, + CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE + ) + : CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE; } } diff --git a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java index b1a5189e48db6..bf733e9f5a00d 100644 --- a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java @@ -451,11 +451,9 @@ public CollectionStatistics collectionStatistics(String field) throws IOExceptio */ @Override public LeafSlice[] slices(List leaves) { - final int target_max_slices = SearchBootstrapSettings.getValueAsInt( - SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, - SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE - ); - return slicesInternal(leaves, target_max_slices); + // For now using the static setting to get the targetMaxSlice value. It will be updated to dynamic mechanism as part of + // https://github.com/opensearch-project/OpenSearch/issues/8870 when lucene changes are available + return slicesInternal(leaves, SearchBootstrapSettings.getTargetMaxSlice()); } public DirectoryReader getDirectoryReader() { @@ -539,15 +537,15 @@ private boolean shouldReverseLeafReaderContexts() { } // package-private for testing - LeafSlice[] slicesInternal(List leaves, int target_max_slices) { + LeafSlice[] slicesInternal(List leaves, int targetMaxSlice) { LeafSlice[] leafSlices; - if (target_max_slices <= 0) { + if (targetMaxSlice <= 0) { // use the default lucene slice calculation leafSlices = super.slices(leaves); logger.debug("Slice count using lucene default [{}]", leafSlices.length); } else { - // use the custom slice calculation based on target_max_slices. It will sort - leafSlices = MaxTargetSliceSupplier.getSlices(leaves, target_max_slices); + // use the custom slice calculation based on targetMaxSlice + leafSlices = MaxTargetSliceSupplier.getSlices(leaves, targetMaxSlice); logger.debug("Slice count using max target slice supplier [{}]", leafSlices.length); } return leafSlices; diff --git a/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java b/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java index 0ee20c02deefc..45feec67a7804 100644 --- a/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java +++ b/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java @@ -21,16 +21,18 @@ * all the leaves based on document count and then assign each leaf in round-robin fashion to the target slice count slices. Based on * experiment results as shared in issue-7358 * we can see this mechanism helps to achieve better tail/median latency over default lucene slice computation. + * + * @opensearch.internal */ public class MaxTargetSliceSupplier { - public static IndexSearcher.LeafSlice[] getSlices(List leaves, int target_max_slice) { - if (target_max_slice <= 0) { - throw new IllegalArgumentException("MaxTargetSliceSupplier called with unexpected slice count of " + target_max_slice); + public static IndexSearcher.LeafSlice[] getSlices(List leaves, int targetMaxSlice) { + if (targetMaxSlice <= 0) { + throw new IllegalArgumentException("MaxTargetSliceSupplier called with unexpected slice count of " + targetMaxSlice); } // slice count should not exceed the segment count - int target_slice_count = Math.min(target_max_slice, leaves.size()); + int targetSliceCount = Math.min(targetMaxSlice, leaves.size()); // Make a copy so we can sort: List sortedLeaves = new ArrayList<>(leaves); @@ -39,23 +41,15 @@ public static IndexSearcher.LeafSlice[] getSlices(List leaves sortedLeaves.sort(Collections.reverseOrder(Comparator.comparingInt(l -> l.reader().maxDoc()))); final List> groupedLeaves = new ArrayList<>(); - for (int i = 0; i < target_slice_count; ++i) { + for (int i = 0; i < targetSliceCount; ++i) { groupedLeaves.add(new ArrayList<>()); } // distribute the slices in round-robin fashion - List group; for (int idx = 0; idx < sortedLeaves.size(); ++idx) { - int currentGroup = idx % target_slice_count; - group = groupedLeaves.get(currentGroup); - group.add(sortedLeaves.get(idx)); + int currentGroup = idx % targetSliceCount; + groupedLeaves.get(currentGroup).add(sortedLeaves.get(idx)); } - IndexSearcher.LeafSlice[] slices = new IndexSearcher.LeafSlice[target_slice_count]; - int upto = 0; - for (List currentLeaf : groupedLeaves) { - slices[upto] = new IndexSearcher.LeafSlice(currentLeaf); - ++upto; - } - return slices; + return groupedLeaves.stream().map(IndexSearcher.LeafSlice::new).toArray(IndexSearcher.LeafSlice[]::new); } } diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java index 769afb1bdf59c..1c999acb2827d 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java @@ -212,10 +212,7 @@ protected final Collection> pluginList(Class