From b2d9b20c25cd80f921069a8e580f46aa338a3e74 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Tue, 10 Oct 2023 20:24:33 -0700 Subject: [PATCH] Changed the default values for ef_search and ef_constrction for enabling better indexing and search latency for vector search Signed-off-by: Navneet Verma --- .../opensearch/knn/bwc/IndexSettingIT.java | 34 ++++++++++++++ .../knn/index/KNNIndexSettingProvider.java | 47 +++++++++++++++++++ .../org/opensearch/knn/index/KNNSettings.java | 5 +- .../org/opensearch/knn/jni/JNIService.java | 3 ++ .../org/opensearch/knn/plugin/KNNPlugin.java | 12 +++++ 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java create mode 100644 src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java diff --git a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java new file mode 100644 index 0000000000..914116c7e2 --- /dev/null +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java @@ -0,0 +1,34 @@ +/* + * 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. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.knn.bwc; + +import org.junit.Assert; +import org.opensearch.knn.index.KNNSettings; + +public class IndexSettingIT extends AbstractRestartUpgradeTestCase { + private static final String TEST_FIELD = "test-field"; + private static final int DIMENSIONS = 5; + + public void testOldIndexSettingsPersistedAfterUpgrade() throws Exception { + if(isRunningAgainstOldCluster()) { + // create index with Old Values + createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS)); + int old_ef_search = Integer.parseInt(getIndexSettingByName(testIndex, + KNNSettings.KNN_ALGO_PARAM_EF_SEARCH)); + Assert.assertEquals(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH.intValue(), old_ef_search); + } else { + int old_ef_search = Integer.parseInt(getIndexSettingByName(testIndex, + KNNSettings.KNN_ALGO_PARAM_EF_SEARCH)); + Assert.assertEquals(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH.intValue(), old_ef_search); + } + } +} diff --git a/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java b/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java new file mode 100644 index 0000000000..7237753afe --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java @@ -0,0 +1,47 @@ +/* + * 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. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.knn.index; + +import lombok.NoArgsConstructor; +import org.opensearch.common.settings.Settings; +import org.opensearch.index.shard.IndexSettingProvider; + +@NoArgsConstructor +public class KNNIndexSettingProvider implements IndexSettingProvider { + + private static final Settings EF_SEARCH_SETTING = Settings.builder().put(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, + KNNSettings.INDEX_KNN_NEW_DEFAULT_ALGO_PARAM_EF_SEARCH).build(); + + /** + * Returns explicitly set default index {@link Settings} for the given index. This should not + * return null. + * + * @param indexName {@link String} name of the index + * @param isDataStreamIndex boolean: index is a datastream index or not + * @param templateAndRequestSettings {@link Settings} + */ + @Override + public Settings getAdditionalIndexSettings(String indexName, boolean isDataStreamIndex, Settings templateAndRequestSettings) { + if(isKNNIndex(templateAndRequestSettings) && isEfSearchNotSetByUser(templateAndRequestSettings)) { + return EF_SEARCH_SETTING; + } + return IndexSettingProvider.super.getAdditionalIndexSettings(indexName, isDataStreamIndex, templateAndRequestSettings); + } + + private boolean isKNNIndex(Settings templateAndRequestSettings) { + return templateAndRequestSettings.getAsBoolean(KNNSettings.KNN_INDEX, false); + } + + private boolean isEfSearchNotSetByUser(Settings templateAndRequestSettings) { + return templateAndRequestSettings.hasValue(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH) == false; + } +} diff --git a/src/main/java/org/opensearch/knn/index/KNNSettings.java b/src/main/java/org/opensearch/knn/index/KNNSettings.java index 4356a06102..72bdae6c67 100644 --- a/src/main/java/org/opensearch/knn/index/KNNSettings.java +++ b/src/main/java/org/opensearch/knn/index/KNNSettings.java @@ -81,7 +81,8 @@ public class KNNSettings { public static final String INDEX_KNN_DEFAULT_SPACE_TYPE = "l2"; public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_M = 16; public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH = 512; - public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION = 512; + public static final Integer INDEX_KNN_NEW_DEFAULT_ALGO_PARAM_EF_SEARCH = 100; + public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION = 100; public static final Integer KNN_DEFAULT_ALGO_PARAM_INDEX_THREAD_QTY = 1; public static final Integer KNN_DEFAULT_CIRCUIT_BREAKER_UNSET_PERCENTAGE = 75; public static final Integer KNN_DEFAULT_MODEL_CACHE_SIZE_LIMIT_PERCENTAGE = 10; // By default, set aside 10% of the JVM for the limit @@ -451,7 +452,7 @@ public static int getEfSearchParam(String index) { .getMetadata() .index(index) .getSettings() - .getAsInt(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, 512); + .getAsInt(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH); } public void setClusterService(ClusterService clusterService) { diff --git a/src/main/java/org/opensearch/knn/jni/JNIService.java b/src/main/java/org/opensearch/knn/jni/JNIService.java index f45fb0c736..7f3ec85f0b 100644 --- a/src/main/java/org/opensearch/knn/jni/JNIService.java +++ b/src/main/java/org/opensearch/knn/jni/JNIService.java @@ -11,6 +11,7 @@ package org.opensearch.knn.jni; +import lombok.extern.log4j.Log4j2; import org.apache.commons.lang.ArrayUtils; import org.opensearch.knn.index.query.KNNQueryResult; import org.opensearch.knn.index.util.KNNEngine; @@ -20,6 +21,7 @@ /** * Service to distribute requests to the proper engine jni service */ +@Log4j2 public class JNIService { /** @@ -32,6 +34,7 @@ public class JNIService { * @param engineName name of engine to build index for */ public static void createIndex(int[] ids, float[][] data, String indexPath, Map parameters, String engineName) { + log.error("index path {}, has the parameters : {}, engine: {}", indexPath, parameters, engineName); if (KNNEngine.NMSLIB.getName().equals(engineName)) { NmslibService.createIndex(ids, data, indexPath, parameters); return; diff --git a/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java b/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java index ea962ecff9..5e74b31059 100644 --- a/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java +++ b/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java @@ -11,9 +11,11 @@ import org.opensearch.core.action.ActionResponse; import org.opensearch.index.codec.CodecServiceFactory; import org.opensearch.index.engine.EngineFactory; +import org.opensearch.index.shard.IndexSettingProvider; import org.opensearch.indices.SystemIndexDescriptor; import org.opensearch.knn.index.KNNCircuitBreaker; import org.opensearch.knn.index.KNNClusterUtil; +import org.opensearch.knn.index.KNNIndexSettingProvider; import org.opensearch.knn.index.query.KNNQueryBuilder; import org.opensearch.knn.index.KNNSettings; import org.opensearch.knn.index.mapper.KNNVectorFieldMapper; @@ -361,4 +363,14 @@ public Settings additionalSettings() { .collect(Collectors.toList()); return Settings.builder().putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), finalSettings).build(); } + + /** + * An {@link IndexSettingProvider} allows hooking in to parts of an index + * lifecycle to provide explicit default settings for newly created indices. Rather than changing + * the default values for an index-level setting, these act as though the setting has been set + * explicitly, but still allow the setting to be overridden by a template or creation request body. + */ + public Collection getAdditionalIndexSettingProviders() { + return List.of(new KNNIndexSettingProvider()); + } }