Adds the replication type index setting, alongside a formal notion of feature flags by kartg · Pull Request #3037 · opensearch-project/OpenSearch

kartg · 2022-04-22T01:16:20Z

Signed-off-by: Kartik Ganesh gkart@amazon.com

Description

This change formalizes the notion of feature flags, and adds a "replication type" setting that will differentiate between document and segment replication, gated by a feature flag.

Since seg-rep is currently an incomplete implementation, the feature flag ensures that the setting is not visible to users without explicitly setting a system property. We can then continue to merge seg-rep related changes from the feature branch to main safely hidden behind the feature flag gate.

The ReplicationType setting is represented as an enum. Its default value is DOCUMENT.

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed 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.

opensearch-ci-bot · 2022-04-22T01:31:47Z

❌ Gradle Check failure 3fdfd117578a5d053ae6e3a389b2cdd35b1f663f
Log 4704

Reports 4704

nknize

🎉 🎉 Love to see the progress toward feature flags!!! Thinking we could maybe add a FeatureFlag utility class as a home for future flags?

nknize · 2022-04-22T02:56:48Z

Maybe we can create a new FeatureFlag utility class?

nknize · 2022-04-22T03:10:08Z

This is odd!!! I sure hope the maven repo endpoint is still accessible and someone didn't blow it away! /cc @mch2

   > Could not find org.apache.lucene:lucene-core:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
   > Could not find org.apache.lucene:lucene-analysis-common:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
   > Could not find org.apache.lucene:lucene-backward-codecs:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
   > Could not find org.apache.lucene:lucene-grouping:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
   > Could not find org.apache.lucene:lucene-highlighter:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
   > Could not find org.apache.lucene:lucene-join:9.2.0-snapshot-f4f1f70.
     Required by:
         project : > project :server
...

nknize · 2022-04-22T03:11:03Z

start gradle check

opensearch-ci-bot · 2022-04-22T03:27:14Z

❌ Gradle Check failure 3fdfd117578a5d053ae6e3a389b2cdd35b1f663f
Log 4710

Reports 4710

nknize · 2022-04-22T04:05:25Z

Maybe try adding

  maven {
    url "https://d1nvenhzbhpy0q.cloudfront.net/snapshots/lucene/"
  }

to code-coverage.gradle? This was recently changed so I don't know if it ever picked up the snapshot repository? /cc @reta can you verify?

reta · 2022-04-22T12:19:48Z

@nknize for some reasons code-coverage.gradle populates own repositories, we could probably remove it and use the ones defined in top level build.gradle

dblock · 2022-04-22T14:34:16Z

#3042

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

nknize · 2022-04-22T19:34:40Z

@kartg I just rebased w/ latest updates to main. 🤞 it passes lucene library retrieval

opensearch-ci-bot · 2022-04-22T20:08:02Z

✅ Gradle Check success ae9fcfd
Log 4730

Reports 4730

In addition to centralizing the feature flag definitions and logic, this also allows us to centralize the checking of feature flags and corresponding addition of feature-flagged index settings into IndexScopedSettings. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg · 2022-04-22T21:05:28Z

Still a WIP:

FeatureFlags needs unit tests
Need to incorporate Adding segment replication setting for index #2904 (comment)

opensearch-ci-bot · 2022-04-22T21:34:28Z

✅ Gradle Check success f9153c4
Log 4737

Reports 4737

Also incorporates PR feedback about the feature flag scope and naming Signed-off-by: Kartik Ganesh <gkart@amazon.com>

opensearch-ci-bot · 2022-04-22T22:29:07Z

❌ Gradle Check failure 8bda19d
Log 4738

Reports 4738

I forgot that valueOf parses the name of the enum and not its value. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg · 2022-04-22T22:56:45Z

+     * is ready for production release, the feature flag can be removed, and the
+     * setting should be moved to {@link #BUILT_IN_INDEX_SETTINGS}.
+     */
+    public static final Map<String, Setting> FEATURE_FLAGGED_INDEX_SETTINGS = Map.of(


Note - Map.of is only available from Java 9+

This shouldn't be an issue since we have to remain 11+ due to lucene 9.

isSegRepEnabled is now computed on the fly by testing the stored ReplicationType value Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg · 2022-04-22T23:02:56Z

+
+import org.opensearch.test.OpenSearchTestCase;
+
+public class FeatureFlagTests extends OpenSearchTestCase {


I realize that these unit tests are rudimentary and don't actually test the isEnabled == true code path for FeatureFlags. I wasn't able to use System.setProperty - it gets flagged as a forbidden API. I would appreciate suggestions on how I could achieve better test coverage.

I pushed a commit that grants write permissions on opensearch.experimental.feature.* so we can test the flags accordingly.

andrross · 2022-04-22T22:57:30Z

+     * Returns true if segment replication is enabled on the index.
+     */
+    public boolean isSegrepEnabled() {
+        return Boolean.TRUE.equals(isSegRepEnabled);


I think this can just be return isSegRepEnabled? Also probably make 'R' uppercase in the method name to be consistent.

sorry, I'll stop commenting until you move this out of draft :)

The logic in this method has changed with the latest commit - IndexSettings now stores the replication type itself rather than storing a boolean.

That said, I do like SegRep better than Segrep so i'll change the method name 😏

Also, feel free to leave early comments - I think this change is just about finalized anyway

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

opensearch-ci-bot · 2022-04-23T00:03:39Z

❌ Gradle Check failure e5c27c9
Log 4739

Reports 4739

opensearch-ci-bot · 2022-04-23T00:17:24Z

❌ Gradle Check failure 5b5a946
Log 4740

Reports 4740

opensearch-ci-bot · 2022-04-23T00:43:14Z

✅ Gradle Check success 2001ed8
Log 4742

Reports 4742

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

nknize

This first version looks good to me! Added a commit to correctly test the feature flags.

nknize · 2022-04-23T05:04:12Z

+     * and false otherwise.
+     */
+    public static boolean isEnabled(String featureFlagName) {
+        return "true".equalsIgnoreCase(System.getProperty(featureFlagName));


Suggested change

return "true".equalsIgnoreCase(System.getProperty(featureFlagName));

return "true".equalsIgnoreCase(System.getProperty(featureFlagName)) || Build.CURRENT.isSnapshot();

It might be nice to auto enable for snapshot builds? Doing this, though, will most certainly fail the FeatureFlag tests the way they are currently written.

nknize · 2022-04-23T06:04:02Z

+
+import org.opensearch.test.OpenSearchTestCase;
+
+public class FeatureFlagTests extends OpenSearchTestCase {


I pushed a commit that grants write permissions on opensearch.experimental.feature.* so we can test the flags accordingly.

opensearch-ci-bot · 2022-04-23T06:31:28Z

❌ Gradle Check failure bb7afbb
Log 4745

Reports 4745

nknize · 2022-04-25T16:24:43Z

Another #1703

REPRODUCE WITH: ./gradlew ':qa:remote-clusters:integTest' --tests "org.opensearch.cluster.remote.test.RemoteClustersIT.testHAProxyModeConnectionWorks" -Dtests.seed=FC5ED4D7E7504EB8 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=no -Dtests.timezone=Etc/GMT-12 -Druntime.java=17

org.opensearch.cluster.remote.test.RemoteClustersIT > testHAProxyModeConnectionWorks FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([FC5ED4D7E7504EB8:FB28011CDCD638E5]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.opensearch.cluster.remote.test.RemoteClustersIT.testHAProxyModeConnectionWorks(RemoteClustersIT.java:125)

nknize · 2022-04-25T16:24:50Z

start gradle check

opensearch-ci-bot · 2022-04-25T16:56:13Z

✅ Gradle Check success bb7afbb
Log 4783

Reports 4783

nknize · 2022-04-25T17:07:57Z

This is great! I like where this PR is at as an incremental step. Especially since it introduces the feature flag mechanism. That will be huge for avoiding those long running feature branches and living in merge hell. I think we should consider enabling all feature flags as the default for snapshot builds but that can be done in a follow up PR after a discussion on if / how we want to do that.

Since this is out of WIP status I think it's ready for your final review @andrross.

kartg · 2022-04-25T17:47:42Z

I think we should consider enabling all feature flags as the default for snapshot builds but that can be done in a follow up PR after a discussion on if / how we want to do that.

I'd prefer to do this in a follow-up PR, after a discussion. Given that we're currently using the feature flag to hide seg-rep as we slowly merge in changes, I'm nervous about having the setting visible by default in snapshot builds, and users assuming the feature is functional. That said, I recognize that the main branch doesn't guarantee stability so it might be acceptable to do this.

… feature flags (#3037) * This change formalizes the notion of feature flags, and adds a "replication type" setting that will differentiate between document and segment replication, gated by a feature flag. Since seg-rep is currently an incomplete implementation, the feature flag ensures that the setting is not visible to users without explicitly setting a system property. We can then continue to merge seg-rep related changes from the feature branch to `main` safely hidden behind the feature flag gate. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Update security policy for testing feature flags Signed-off-by: Nicholas Walter Knize <nknize@apache.org> Co-authored-by: Nicholas Walter Knize <nknize@apache.org> (cherry picked from commit ee7b731)

… feature flags (#3037) (#3071) * This change formalizes the notion of feature flags, and adds a "replication type" setting that will differentiate between document and segment replication, gated by a feature flag. Since seg-rep is currently an incomplete implementation, the feature flag ensures that the setting is not visible to users without explicitly setting a system property. We can then continue to merge seg-rep related changes from the feature branch to `main` safely hidden behind the feature flag gate. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Update security policy for testing feature flags Signed-off-by: Nicholas Walter Knize <nknize@apache.org> Co-authored-by: Nicholas Walter Knize <nknize@apache.org> (cherry picked from commit ee7b731) Co-authored-by: Kartik Ganesh <gkart@amazon.com>

nknize reviewed Apr 22, 2022

View reviewed changes

nknize added feature New feature or request v2.1.0 Issues and PRs related to version 2.1.0 v3.0.0 Issues and PRs related to version 3.0.0 labels Apr 22, 2022

Add segment replication index setting, gated by feature flag

ae9fcfd

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

nknize force-pushed the merge-segrep-setting-to-main branch from 3fdfd11 to ae9fcfd Compare April 22, 2022 19:33

reta mentioned this pull request Apr 22, 2022

Revert "Support task resource tracking in OpenSearch" #3046

Merged

andrross reviewed Apr 22, 2022

View reviewed changes

Comment thread server/src/main/java/org/opensearch/common/util/FeatureFlags.java Outdated

Change replication index setting from boolean to Enum

8bda19d

Also incorporates PR feedback about the feature flag scope and naming Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg added 2 commits April 22, 2022 15:42

Simplifying the code in ReplicationType

25901f4

I forgot that valueOf parses the name of the enum and not its value. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

Added basic unit tests for the FeatureFlags utility class

e5c27c9

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg commented Apr 22, 2022

View reviewed changes

IndexSettings now stores the ReplicationType enum instead of a boolean

5b5a946

isSegRepEnabled is now computed on the fly by testing the stored ReplicationType value Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg commented Apr 22, 2022

View reviewed changes

kartg changed the title ~~Add segment replication index setting, gated by feature flag~~ Adds the replication type index setting, alongside a formal notion of feature flags Apr 22, 2022

andrross reviewed Apr 22, 2022

View reviewed changes

Tiny improvement to IndexSettings method name

2001ed8

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg marked this pull request as ready for review April 22, 2022 23:30

kartg requested review from a team and reta as code owners April 22, 2022 23:30

Update security policy for testing feature flags

bb7afbb

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

nknize approved these changes Apr 23, 2022

View reviewed changes

nknize added Indexing & Search backport 2.x Backport to 2.x branch labels Apr 25, 2022

nknize requested a review from andrross April 25, 2022 17:08

nknize mentioned this pull request Apr 25, 2022

Adding segment replication setting for index #2904

Closed

5 tasks

andrross approved these changes Apr 25, 2022

View reviewed changes

kartg merged commit ee7b731 into opensearch-project:main Apr 25, 2022

opensearch-trigger-bot bot mentioned this pull request Apr 25, 2022

[Backport 2.x] Adds the replication type index setting, alongside a formal notion of feature flags #3071

Merged

kartg deleted the merge-segrep-setting-to-main branch April 25, 2022 17:53

dblock added the documentation pending Tracks issues which have PRs merged but documentation changes pending label Apr 25, 2022

dreamer-89 mentioned this pull request Jul 7, 2022

[Segment Replication] Add test support for segrep integration tests #3818

Closed

Rishikesh1159 mentioned this pull request Apr 3, 2023

[Segment Replication] Promote Segment Replication out of Feature Flag #6951

Closed


		import org.opensearch.test.OpenSearchTestCase;

		public class FeatureFlagTests extends OpenSearchTestCase {

	return "true".equalsIgnoreCase(System.getProperty(featureFlagName));
	return "true".equalsIgnoreCase(System.getProperty(featureFlagName)) \|\| Build.CURRENT.isSnapshot();

Conversation

kartg commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Apr 22, 2022

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize commented Apr 22, 2022

Uh oh!

nknize commented Apr 22, 2022

Uh oh!

opensearch-ci-bot commented Apr 22, 2022

Uh oh!

nknize commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reta commented Apr 22, 2022

Uh oh!

dblock commented Apr 22, 2022

Uh oh!

nknize commented Apr 22, 2022

Uh oh!

opensearch-ci-bot commented Apr 22, 2022

Uh oh!

kartg commented Apr 22, 2022

Uh oh!

opensearch-ci-bot commented Apr 22, 2022

Uh oh!

Uh oh!

opensearch-ci-bot commented Apr 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartg Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Apr 23, 2022

Uh oh!

opensearch-ci-bot commented Apr 23, 2022

Uh oh!

opensearch-ci-bot commented Apr 23, 2022

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Apr 23, 2022

Uh oh!

nknize commented Apr 25, 2022

Uh oh!

nknize commented Apr 25, 2022

Uh oh!

opensearch-ci-bot commented Apr 25, 2022

Uh oh!

nknize commented Apr 25, 2022

Uh oh!

kartg commented Apr 25, 2022

Uh oh!

Reviewers

kartg commented Apr 22, 2022 •

edited

Loading

nknize commented Apr 22, 2022 •

edited

Loading

kartg Apr 22, 2022 •

edited

Loading