-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds the replication type index setting, alongside a formal notion of feature flags #3037
Changes from all commits
ae9fcfd
f9153c4
8bda19d
25901f4
e5c27c9
5b5a946
2001ed8
bb7afbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,32 @@ | ||||||
/* | ||||||
* 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. | ||||||
*/ | ||||||
|
||||||
package org.opensearch.common.util; | ||||||
|
||||||
/** | ||||||
* Utility class to manage feature flags. Feature flags are system properties that must be set on the JVM. | ||||||
* These are used to gate the visibility/availability of incomplete features. Fore more information, see | ||||||
* https://featureflags.io/feature-flag-introduction/ | ||||||
*/ | ||||||
public class FeatureFlags { | ||||||
|
||||||
/** | ||||||
* Gates the visibility of the index setting that allows changing of replication type. | ||||||
* Once the feature is ready for production release, this feature flag can be removed. | ||||||
*/ | ||||||
public static final String REPLICATION_TYPE = "opensearch.experimental.feature.replication_type.enabled"; | ||||||
|
||||||
/** | ||||||
* Used to test feature flags whose values are expected to be booleans. | ||||||
* This method returns true if the value is "true" (case-insensitive), | ||||||
* and false otherwise. | ||||||
*/ | ||||||
public static boolean isEnabled(String featureFlagName) { | ||||||
return "true".equalsIgnoreCase(System.getProperty(featureFlagName)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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. |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
package org.opensearch.indices.replication.common; | ||
|
||
/** | ||
* Enumerates the types of replication strategies supported by OpenSearch. | ||
* For more information, see https://github.com/opensearch-project/OpenSearch/issues/1694 | ||
*/ | ||
public enum ReplicationType { | ||
|
||
DOCUMENT, | ||
SEGMENT; | ||
|
||
public static ReplicationType parseString(String replicationType) { | ||
try { | ||
return ReplicationType.valueOf(replicationType); | ||
} catch (IllegalArgumentException e) { | ||
throw new IllegalArgumentException("Could not parse ReplicationStrategy for [" + replicationType + "]"); | ||
} catch (NullPointerException npe) { | ||
// return a default value for null input | ||
return DOCUMENT; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
package org.opensearch.common.util; | ||
|
||
import org.junit.BeforeClass; | ||
import org.opensearch.common.SuppressForbidden; | ||
import org.opensearch.test.OpenSearchTestCase; | ||
|
||
import java.security.AccessController; | ||
import java.security.PrivilegedAction; | ||
|
||
public class FeatureFlagTests extends OpenSearchTestCase { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize that these unit tests are rudimentary and don't actually test the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a commit that grants write permissions on |
||
|
||
@SuppressForbidden(reason = "sets the feature flag") | ||
@BeforeClass | ||
public static void enableFeature() { | ||
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.REPLICATION_TYPE, "true")); | ||
} | ||
|
||
public void testReplicationTypeFeatureFlag() { | ||
String replicationTypeFlag = FeatureFlags.REPLICATION_TYPE; | ||
assertNotNull(System.getProperty(replicationTypeFlag)); | ||
assertTrue(FeatureFlags.isEnabled(replicationTypeFlag)); | ||
} | ||
|
||
public void testMissingFeatureFlag() { | ||
String testFlag = "missingFeatureFlag"; | ||
assertNull(System.getProperty(testFlag)); | ||
assertFalse(FeatureFlags.isEnabled(testFlag)); | ||
} | ||
|
||
public void testNonBooleanFeatureFlag() { | ||
String javaVersionProperty = "java.version"; | ||
assertNotNull(System.getProperty(javaVersionProperty)); | ||
assertFalse(FeatureFlags.isEnabled(javaVersionProperty)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note -
Map.of
is only available from Java 9+There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be an issue since we have to remain 11+ due to lucene 9.