Adding segment replication setting for index #2904
Conversation
Signed-off-by: Poojita Raj <[email protected]>
There was a problem hiding this comment.
Did we decide to feature flag segrep for 2x?
If so do we want to wrap this setting in a feature flag and only enable if the flag is set? /cc @mch2 @kartg
public static final boolean SEGREP_FEATURE_FLAG_ENABLED = "true".equals(System.getProperty("opensearch.segment_replication_feature_flag_enabled"));
Hey @nknize. Just to be clear we are talking about merging to main in anticipation of 3x, with an option to pull into 2x when the feature is ready to experiment with? I would rather not backport these changes immediately into 2x before the feature is useable. We are hoping to go to main with these changes to remove our long running feature branch, but we have issues like failover and some refactoring that would need to be addressed before it is ready for experimentation. As far as the feature flag, what benefit would this give over the new setting? Is it that a user would have the additional step/warning of having to set it as a jvm parameter over a setting during index creation? |
@nknize I agree with Marc about not backporting anything to 2.x just yet, but I have a general question... Is there a standard practice for using a "feature flag" that you described above? I don't see the benefit of needing to both set a Java system property and a new setting a configuration file, since the feature would be disabled by default in any case. |
Exactly.. the way that I would approach this is to bring into main w/ the feature flag. If we collectively decide to backport to 2x, we can open a backport PR with the feature flag. If we then collectively decide the feature is at a mature enough state for 3x, we open a follow on PR to remove the feature flag in main only. This gives us a nice progressive maturation mechanism with off ramps along the way.
With feature flags the index setting isn't even available in the API w/o the Follow up: We can commit w/ the feature flag enabled in all test module's build.gradle file. That way tests run with it enabled. We can also enable the feature flag in nightly snapshots. You can continue to safely develop the feature in main w/o carrying the long running feature branch and dealing w/ merge conflicts. 💀 🔫 |
| @@ -260,6 +260,17 @@ public Iterator<Setting<?>> settings() { | |||
| Property.IndexScope | |||
| ); | |||
|
|
|||
| public static final String SETTING_SEGMENT_REPLICATION = "index.replication.segment_replication"; | |||
There was a problem hiding this comment.
Nitpick, but I'd consider making this an enum setting, like index.replication.type=["DOCUMENT"|"SEGMENT"], defaulting to "DOCUMENT". It conveys a little more information that the replication methods are mutually exclusive for a given index, and also gives a name to "document replication" instead of being implicit.
Several approaches; I linked one in my response to @mch2 and here's another good resource
Think of the feature flag like a compile time Macro... You can safely wrap the in-work feature and prevent it from ever being exposed until the project deems ready; whereas the index setting commits it as a feature. The index setting is the power button, the feature flag is the locked cover to the power button. |
|
This makes sense to me, thanks @nknize. So our feature flag is not setting a default value of the setting, but its an additional gate to flipping it on if the setting is passed. We'll need to think about how to conditionally bring in the additional logic and turn it on. We don't have isolated components that get installed/uninstalled based on the flag on our feature branch. There are many updates to existing classes with conditional logic. |
Yes, closing in favor of that one |
Signed-off-by: Poojita Raj [email protected]
Description
At the index level, we add in a new setting to enable segment replication. It is set to false by default, in which case the index uses the document replication strategy.
This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch.
The breakdown of the plan to merge to main is detailed here: #2355
For added context on segment replication - here's the design proposal #2229
Check List
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.