Made hybrid search active by default by flipping feature flag default

Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 174f2c9)
martin-gaievski · Sep 7, 2023 · 4cc0cdc · 4cc0cdc
1 parent 5bf36ed
commit 4cc0cdc
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 26 deletions.
diff --git a/build.gradle b/build.gradle
@@ -253,10 +253,6 @@ testClusters.integTest {
     // Increase heap size from default of 512mb to 1gb. When heap size is 512mb, our integ tests sporadically fail due
     // to ml-commons memory circuit breaker exception
     jvmArgs("-Xms1g", "-Xmx1g")
-
-    // enable features for testing
-    // hybrid search
-    systemProperty('plugins.neural_search.hybrid_search_enabled', 'true')
 }
 
 // Remote Integration Tests

diff --git a/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java b/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java
@@ -5,7 +5,7 @@
 
 package org.opensearch.neuralsearch.plugin;
 
-import static org.opensearch.neuralsearch.settings.NeuralSearchSettings.NEURAL_SEARCH_HYBRID_SEARCH_ENABLED;
+import static org.opensearch.neuralsearch.settings.NeuralSearchSettings.NEURAL_SEARCH_HYBRID_SEARCH_DISABLED;
 
 import java.util.Arrays;
 import java.util.Collection;
@@ -99,16 +99,18 @@ public Map<String, Processor.Factory> getProcessors(Processor.Parameters paramet
 
     @Override
     public Optional<QueryPhaseSearcher> getQueryPhaseSearcher() {
-        if (FeatureFlags.isEnabled(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED.getKey())) {
-            log.info("Registering hybrid query phase searcher with feature flag [{}]", NEURAL_SEARCH_HYBRID_SEARCH_ENABLED.getKey());
-            return Optional.of(new HybridQueryPhaseSearcher());
+        // we're using "is_disabled" flag as there are no proper implementation of FeatureFlags.isDisabled(). Both
+        // cases when flag is not set or it is "false" are interpretted in the same way. In such case core is reading
+        // the actual value from settings.
+        if (FeatureFlags.isEnabled(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey())) {
+            log.info(
+                "Not registering hybrid query phase searcher because feature flag [{}] is disabled",
+                NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey()
+            );
+            return Optional.empty();
         }
-        log.info(
-            "Not registering hybrid query phase searcher because feature flag [{}] is disabled",
-            NEURAL_SEARCH_HYBRID_SEARCH_ENABLED.getKey()
-        );
-        // we want feature be disabled by default due to risk of colliding and breaking concurrent search in core
-        return Optional.empty();
+        log.info("Registering hybrid query phase searcher with feature flag [{}]", NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey());
+        return Optional.of(new HybridQueryPhaseSearcher());
     }
 
     @Override
@@ -123,6 +125,6 @@ public Map<String, org.opensearch.search.pipeline.Processor.Factory<SearchPhaseR
 
     @Override
     public List<Setting<?>> getSettings() {
-        return List.of(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED);
+        return List.of(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED);
     }
 }
diff --git a/src/main/java/org/opensearch/neuralsearch/settings/NeuralSearchSettings.java b/src/main/java/org/opensearch/neuralsearch/settings/NeuralSearchSettings.java
@@ -21,8 +21,8 @@ public final class NeuralSearchSettings {
      * Currently query phase searcher added with hybrid search will conflict with concurrent search in core.
      * Once that problem is resolved this feature flag can be removed.
      */
-    public static final Setting<Boolean> NEURAL_SEARCH_HYBRID_SEARCH_ENABLED = Setting.boolSetting(
-        "plugins.neural_search.hybrid_search_enabled",
+    public static final Setting<Boolean> NEURAL_SEARCH_HYBRID_SEARCH_DISABLED = Setting.boolSetting(
+        "plugins.neural_search.hybrid_search_disabled",
         false,
         Setting.Property.NodeScope
     );

diff --git a/src/test/java/org/opensearch/neuralsearch/plugin/NeuralSearchTests.java b/src/test/java/org/opensearch/neuralsearch/plugin/NeuralSearchTests.java
@@ -38,18 +38,18 @@ public void testQuerySpecs() {
 
     public void testQueryPhaseSearcher() {
         NeuralSearch plugin = new NeuralSearch();
-        Optional<QueryPhaseSearcher> queryPhaseSearcher = plugin.getQueryPhaseSearcher();
-
-        assertNotNull(queryPhaseSearcher);
-        assertTrue(queryPhaseSearcher.isEmpty());
-
-        initFeatureFlags();
-
         Optional<QueryPhaseSearcher> queryPhaseSearcherWithFeatureFlagDisabled = plugin.getQueryPhaseSearcher();
 
         assertNotNull(queryPhaseSearcherWithFeatureFlagDisabled);
         assertFalse(queryPhaseSearcherWithFeatureFlagDisabled.isEmpty());
         assertTrue(queryPhaseSearcherWithFeatureFlagDisabled.get() instanceof HybridQueryPhaseSearcher);
+
+        initFeatureFlags();
+
+        Optional<QueryPhaseSearcher> queryPhaseSearcher = plugin.getQueryPhaseSearcher();
+
+        assertNotNull(queryPhaseSearcher);
+        assertTrue(queryPhaseSearcher.isEmpty());
     }
 
     public void testProcessors() {

diff --git a/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java b/src/test/java/org/opensearch/neuralsearch/query/OpenSearchQueryTestCase.java
@@ -8,7 +8,7 @@
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static java.util.stream.Collectors.toList;
-import static org.opensearch.neuralsearch.settings.NeuralSearchSettings.NEURAL_SEARCH_HYBRID_SEARCH_ENABLED;
+import static org.opensearch.neuralsearch.settings.NeuralSearchSettings.NEURAL_SEARCH_HYBRID_SEARCH_DISABLED;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -227,6 +227,6 @@ public float getMaxScore(int upTo) {
 
     @SuppressForbidden(reason = "manipulates system properties for testing")
     protected static void initFeatureFlags() {
-        System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED.getKey(), "true");
+        System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getKey(), "true");
     }
 }