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 000000000..1d27f7200 --- /dev/null +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java @@ -0,0 +1,40 @@ +/* + * 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, true)); + 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, true)); + Assert.assertEquals(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH.intValue(), old_ef_search); + deleteKNNIndex(testIndex); + } + } + + // private void assertEfSearchOldDefaultValue(String indexName) { + // if (Version.fromString(getBWCVersion().get()).onOrAfter(Version.V_2_10_0)) { + // + // } + // + // } +} 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 000000000..cd60e5698 --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java @@ -0,0 +1,48 @@ +/* + * 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 4356a0610..72bdae6c6 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 f45fb0c73..7f3ec85f0 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 ea962ecff..5e74b3105 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()); + } } diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 1c754a5c7..d872db4ba 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -14,6 +14,7 @@ import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.core.xcontent.DeprecationHandler; import org.opensearch.core.xcontent.MediaType; +import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.MatchAllQueryBuilder; @@ -49,6 +50,7 @@ import javax.management.remote.JMXServiceURL; import java.io.IOException; +import java.io.InputStream; import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; @@ -632,11 +634,27 @@ protected int getTotalGraphsInCache() throws IOException { * Get specific Index setting value from response */ protected String getIndexSettingByName(String indexName, String settingName) throws IOException { - @SuppressWarnings("unchecked") - Map settings = (Map) ((Map) getIndexSettings(indexName).get(indexName)).get( - "settings" - ); - return (String) settings.get(settingName); + return getIndexSettingByName(indexName, settingName, false); + } + + protected String getIndexSettingByName(String indexName, String settingName, boolean includeDefaults) throws IOException { + Request request = new Request("GET", "/" + indexName + "/_settings"); + if (includeDefaults) { + request.addParameter("include_defaults", "true"); + } + request.addParameter("flat_settings", "true"); + Response response = client().performRequest(request); + try (InputStream is = response.getEntity().getContent()) { + Map settings = (Map) XContentHelper.convertToMap(MediaTypeRegistry.JSON.xContent(), is, true) + .get(indexName); + Map defaultSettings = new HashMap<>(); + if (includeDefaults) { + defaultSettings = (Map) settings.get("defaults"); + } + Map userSettings = (Map) settings.get("settings"); + // logger.error("Default settings: {}, index settings {}", defaultSettings, userSettings); + return (String) (userSettings.get(settingName) == null ? defaultSettings.get(settingName) : userSettings.get(settingName)); + } } protected void createModelSystemIndex() throws IOException {