Add feature flag for QueryPhaseSearcher #214

martin-gaievski · 2023-07-12T16:50:15Z

Description

Added feature flag neural_search_hybrid_search_enabled for query phase searcher. This is required as a safeguard in case concurrent search feature is enabled and another implementation of query phase searcher is registered.
This new flag can be set as system property or in opensearch.yml (with use of transport.features), by default flag is false (feature is disabled). Enable flag for integ tests.

Issues Resolved

#194

Check List

New functionality includes testing.
- All tests pass
New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski · 2023-07-12T16:52:12Z

src/main/java/org/opensearch/neuralsearch/util/PluginFeatureFlags.java

+     * and false otherwise.
+     * Checks alternative flag names as they may be different for plugins
+     */
+    public static boolean isEnabled(String featureFlagName) {


we need to try "transport" feature prefix, as this is the way core adding features that are set in plugins.

Then why both? Shouldn't we only check with "transport" feature prefix?

I wanted to encapsulate that logic so client can do only one call. Otherwise on a client we need to do two calls:
featureFlags.isEnabled(flag) || customFeatureFlags.isEnabled(flag). Depending on scenario one feature flag can be set in one form or another (with or without prefix)

I still think it is client's responsibility to pass correct key. In this case transport.features.neural_search_hybrid_search_enabled.

There is no such case that neural_search_hybrid_search_enabled can be enabled by setting neural_search_hybrid_search_enabled as true but only by setting transport.features.neural_search_hybrid_search_enabled as true I believe?

I see your point, it can be that flag is set in two places and as we read it with OR and one that set in plugin is always TRUE then we cannot set it to FALSE ever. I'll rework to only use one name or another, in fact it should be always with the prefix except the definition.

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

heemin32 · 2023-07-12T19:51:46Z

src/main/java/org/opensearch/neuralsearch/util/PluginFeatureFlags.java

+     * and false otherwise.
+     * Checks alternative flag names as they may be different for plugins
+     */
+    public static boolean isEnabled(String featureFlagName) {


Suggested change

public static boolean isEnabled(String featureFlagName) {

public static boolean isEnabled(final String featureFlagName) {

heemin32 · 2023-07-12T19:54:10Z

src/main/java/org/opensearch/neuralsearch/util/PluginFeatureFlags.java

+     * and false otherwise.
+     * Checks alternative flag names as they may be different for plugins
+     */
+    public static boolean isEnabled(String featureFlagName) {


Then why both? Shouldn't we only check with "transport" feature prefix?

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

codecov · 2023-07-12T20:39:02Z

Codecov Report

Merging #214 (86b51f5) into feature/normalization (7dda0c5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@                     Coverage Diff                     @@
##             feature/normalization     #214      +/-   ##
===========================================================
+ Coverage                    84.91%   84.95%   +0.04%     
- Complexity                     215      216       +1     
===========================================================
  Files                           15       15              
  Lines                          676      678       +2     
  Branches                       109      109              
===========================================================
+ Hits                           574      576       +2     
  Misses                          60       60              
  Partials                        42       42

Impacted Files	Coverage Δ
...nsearch/neuralsearch/query/HybridQueryBuilder.java	`72.64% <ø> (ø)`
...nsearch/neuralsearch/query/NeuralQueryBuilder.java	`89.01% <ø> (ø)`
...g/opensearch/neuralsearch/plugin/NeuralSearch.java	`77.77% <100.00%> (+6.34%)`	⬆️

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

heemin32 · 2023-07-12T22:41:55Z

src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java

@@ -50,9 +52,14 @@ public class NeuralSearch extends Plugin implements ActionPlugin, SearchPlugin,
     * Gates the functionality of hybrid search
     * 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.
+     * Key is the name string, value is key + transport feature specific prefix, prefix is added by core when we register feature
+     * We need to write and read by the value, key is only for definition


I think this previous comment having a code link to core was good and would be nice to have it here.
https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/plugins/PluginsService.java#L277

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

navneet1v · 2023-07-13T17:39:04Z

This new flag can be set as system property or in opensearch.yml (with use of transport.features), by default flag is true (or enabled).

Why we want to make it by default true? This is strange? Can we make this false for now. and then take a decision what needs to be done.

martin-gaievski · 2023-07-13T17:49:34Z

This new flag can be set as system property or in opensearch.yml (with use of transport.features), by default flag is true (or enabled).

Why we want to make it by default true? This is strange? Can we make this false for now. and then take a decision what needs to be done.

We can only make the feature "true" with this approach, but I think you mean it's not about the exact value, but we need to interpret that default "true" as feature is disabled, is that what you mean? In other words user need to opt-in explicitly, rather that opt-out with current approach

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

navneet1v · 2023-07-14T02:01:02Z

src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java

    @VisibleForTesting
-    public static final Pair<String, String> NEURAL_SEARCH_HYBRID_SEARCH_DISABLED = Pair.of(
-        "neural_search_hybrid_search_disabled",
-        String.join(".", TransportSettings.FEATURE_PREFIX, "neural_search_hybrid_search_disabled")
-    );
+    public static final String NEURAL_SEARCH_HYBRID_SEARCH_ENABLED = "neural_search_hybrid_search_enabled";


The variable is already public we can remove @VisbleForTesting

I made it public only because it's required for tests otherwise private will be enough, my understanding is that @VisbleForTesting is required in this case to mark that it's intentional.

oh.. sorry my bad. I read it wrong then

navneet1v · 2023-07-14T02:02:14Z

src/test/java/org/opensearch/neuralsearch/plugin/NeuralSearchTests.java

@@ -42,15 +42,15 @@ public void testQueryPhaseSearcher() {
        assertNotNull(queryPhaseSearcher);
        assertTrue(queryPhaseSearcher.isEmpty());

-        System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_DISABLED.getValue(), "true");
+        System.setProperty(NEURAL_SEARCH_HYBRID_SEARCH_ENABLED, "true");


should we set this property in base class?

and why test are extending OpenSearchTestCase, there is should be a base Neural Search test class

I was thinking that for some unit tests we may want to disable it, that would be complex if it's set in base class.

I'll make setting in a method and will call it in a setup phase for this test class.

navneet1v · 2023-07-14T02:04:27Z

happy to see all checks succeeding. This is happening after a long time

navneet1v · 2023-07-14T02:25:36Z

@martin-gaievski tests are failing.

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

* Adding hybrid_search_enabled settings Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Adding hybrid_search_enabled settings

1fa7e78

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski added the skip-changelog label Jul 12, 2023

martin-gaievski commented Jul 12, 2023

View reviewed changes

martin-gaievski force-pushed the add-feature-flag branch from 80baad0 to 2933240 Compare July 12, 2023 17:12

Add feature flag at plugin level

4985c33

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the add-feature-flag branch from 2933240 to 4985c33 Compare July 12, 2023 17:21

Adopt existing tests to latest changes in knn

3fabc54

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski marked this pull request as ready for review July 12, 2023 18:44

martin-gaievski requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, sean-zheng-amazon, model-collapse, wujunshen, zane-neo, ylwu-amzn and jngz-es as code owners July 12, 2023 18:44

martin-gaievski changed the title ~~Add feature flag~~ Add feature flag for QueryPhaseSearcher Jul 12, 2023

heemin32 reviewed Jul 12, 2023

View reviewed changes

Making all method arguments final

55780e6

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Refactor feature name, remove plugin feature flags class

5142d3f

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski requested a review from heemin32 July 12, 2023 22:37

heemin32 reviewed Jul 12, 2023

View reviewed changes

heemin32 approved these changes Jul 12, 2023

View reviewed changes

Adopt recent refactoring in core

c1356cf

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the add-feature-flag branch from 9e9f932 to c1356cf Compare July 12, 2023 22:45

naveentatikonda approved these changes Jul 13, 2023

View reviewed changes

Invert feature flag meaning, disable feature by default

2617ca4

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the add-feature-flag branch from 58af228 to 620aadf Compare July 13, 2023 21:38

Fixed more copilation errors after latest changes in core

a63b983

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the add-feature-flag branch from 620aadf to a63b983 Compare July 13, 2023 21:42

Remove setting of flag from plugin, make false default value

a6c52bb

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

navneet1v reviewed Jul 14, 2023

View reviewed changes

Minor refactoring in uni tests

86b51f5

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the add-feature-flag branch from 267a716 to 86b51f5 Compare July 14, 2023 02:26

martin-gaievski requested a review from navneet1v July 14, 2023 02:31

navneet1v approved these changes Jul 14, 2023

View reviewed changes

martin-gaievski merged commit c4d0a0c into opensearch-project:feature/normalization Jul 14, 2023

martin-gaievski added a commit that referenced this pull request Aug 3, 2023

Add feature flag for QueryPhaseSearcher (#214)

d87b63e

* Adding hybrid_search_enabled settings Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski mentioned this pull request Aug 3, 2023

Added Score Normalization and Combination feature #241

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flag for QueryPhaseSearcher #214

Add feature flag for QueryPhaseSearcher #214

martin-gaievski commented Jul 12, 2023 •

edited

Loading

martin-gaievski Jul 12, 2023

heemin32 Jul 12, 2023 •

edited

Loading

martin-gaievski Jul 12, 2023

heemin32 Jul 12, 2023

martin-gaievski Jul 12, 2023

heemin32 Jul 12, 2023

martin-gaievski Jul 12, 2023

heemin32 Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

heemin32 Jul 12, 2023

navneet1v commented Jul 13, 2023

martin-gaievski commented Jul 13, 2023

navneet1v Jul 14, 2023

martin-gaievski Jul 14, 2023 •

edited

Loading

navneet1v Jul 14, 2023

navneet1v Jul 14, 2023

martin-gaievski Jul 14, 2023

navneet1v Jul 14, 2023

navneet1v commented Jul 14, 2023

navneet1v commented Jul 14, 2023

	public static boolean isEnabled(String featureFlagName) {
	public static boolean isEnabled(final String featureFlagName) {

Add feature flag for QueryPhaseSearcher #214

Add feature flag for QueryPhaseSearcher #214

Conversation

martin-gaievski commented Jul 12, 2023 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

heemin32 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

navneet1v commented Jul 13, 2023

martin-gaievski commented Jul 13, 2023

Choose a reason for hiding this comment

martin-gaievski Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Jul 14, 2023

navneet1v commented Jul 14, 2023

martin-gaievski commented Jul 12, 2023 •

edited

Loading

heemin32 Jul 12, 2023 •

edited

Loading

heemin32 Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

martin-gaievski Jul 14, 2023 •

edited

Loading