From ce89a52b365d7ce14bd22296ddd0e9fe000b6ed4 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 13:30:36 -0700 Subject: [PATCH 01/10] Refactor FeatureFlags.java. - Remove internal immutable `settings` in favor of `ConcurrentHashMap`. - Move functionality to internal `FeatureFlagsImpl` class. Expose public api of `FeatureFlagsImpl` in FeatureFlags. Expose test api of `FeatureFlagsImpl` in FeatureFlags.TestUtils. - Read and set JVM system properties once on `initializeFeatureFlags`. Remove JVM system properties check from `isEnabled`. - Add `FlagLock` in `TestUtils` to maintain a lock for each feature flag. - Add helper functions to set & access feature flags in a thread safe way. `TestUtils.with(, () -> {})` to execute crtical sections. `New FlagLock()` for fine grained control. Signed-off-by: Finn Carroll --- .../opensearch/common/util/FeatureFlags.java | 293 ++++++++++++++---- 1 file changed, 227 insertions(+), 66 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 4ff81cf0c1c96..df78487f5c459 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -12,43 +12,52 @@ import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; -import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; /** - * 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. For more information, see + * Feature flags are used to gate the visibility/availability of incomplete features. For more information, see * https://featureflags.io/feature-flag-introduction/ - * + * Due to their specific use case, feature flag settings have several additional properties enforced by convention and code: + * - Feature flags are boolean settings. + * - Feature flags are static settings. + * - Feature flags are globally available. + * - Feature flags are configurable by JVM system properties with setting key. * @opensearch.internal */ public class FeatureFlags { + // Prefixes public for testing + public static final String OS_EXPERIMENTAL_PREFIX = "opensearch.experimental."; + public static final String FEATURE_FLAG_PREFIX = OS_EXPERIMENTAL_PREFIX + "feature."; + /** * Gates the visibility of the remote store to docrep migration. */ - public static final String REMOTE_STORE_MIGRATION_EXPERIMENTAL = "opensearch.experimental.feature.remote_store.migration.enabled"; + public static final String REMOTE_STORE_MIGRATION_EXPERIMENTAL = FEATURE_FLAG_PREFIX + "remote_store.migration.enabled"; /** * Gates the ability for Searchable Snapshots to read snapshots that are older than the * guaranteed backward compatibility for OpenSearch (one prior major version) on a best effort basis. */ - public static final String SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY = - "opensearch.experimental.feature.searchable_snapshot.extended_compatibility.enabled"; + public static final String SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY = FEATURE_FLAG_PREFIX + + "searchable_snapshot.extended_compatibility.enabled"; /** * Gates the functionality of extensions. * Once the feature is ready for production release, this feature flag can be removed. */ - public static final String EXTENSIONS = "opensearch.experimental.feature.extensions.enabled"; + public static final String EXTENSIONS = FEATURE_FLAG_PREFIX + "extensions.enabled"; /** * Gates the functionality of telemetry framework. */ - public static final String TELEMETRY = "opensearch.experimental.feature.telemetry.enabled"; + public static final String TELEMETRY = FEATURE_FLAG_PREFIX + "telemetry.enabled"; /** * Gates the optimization of datetime formatters caching along with change in default datetime formatter. */ - public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled"; + public static final String DATETIME_FORMATTER_CACHING = OS_EXPERIMENTAL_PREFIX + "optimization.datetime_formatter_caching.enabled"; /** * Gates the functionality of warm index having the capability to store data remotely. @@ -59,9 +68,9 @@ public class FeatureFlags { /** * Gates the functionality of background task execution. */ - public static final String BACKGROUND_TASK_EXECUTION_EXPERIMENTAL = "opensearch.experimental.feature.task.background.enabled"; + public static final String BACKGROUND_TASK_EXECUTION_EXPERIMENTAL = FEATURE_FLAG_PREFIX + "task.background.enabled"; - public static final String READER_WRITER_SPLIT_EXPERIMENTAL = "opensearch.experimental.feature.read.write.split.enabled"; + public static final String READER_WRITER_SPLIT_EXPERIMENTAL = FEATURE_FLAG_PREFIX + "read.write.split.enabled"; public static final Setting REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting( REMOTE_STORE_MIGRATION_EXPERIMENTAL, @@ -95,13 +104,13 @@ public class FeatureFlags { * Gates the functionality of star tree index, which improves the performance of search * aggregations. */ - public static final String STAR_TREE_INDEX = "opensearch.experimental.feature.composite_index.star_tree.enabled"; + public static final String STAR_TREE_INDEX = FEATURE_FLAG_PREFIX + "composite_index.star_tree.enabled"; public static final Setting STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, false, Property.NodeScope); /** * Gates the functionality of application based configuration templates. */ - public static final String APPLICATION_BASED_CONFIGURATION_TEMPLATES = "opensearch.experimental.feature.application_templates.enabled"; + public static final String APPLICATION_BASED_CONFIGURATION_TEMPLATES = FEATURE_FLAG_PREFIX + "application_templates.enabled"; public static final Setting APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING = Setting.boolSetting( APPLICATION_BASED_CONFIGURATION_TEMPLATES, false, @@ -111,86 +120,238 @@ public class FeatureFlags { /** * Gates the functionality of ApproximatePointRangeQuery where we approximate query results. */ - public static final String APPROXIMATE_POINT_RANGE_QUERY = "opensearch.experimental.feature.approximate_point_range_query.enabled"; + public static final String APPROXIMATE_POINT_RANGE_QUERY = FEATURE_FLAG_PREFIX + "approximate_point_range_query.enabled"; public static final Setting APPROXIMATE_POINT_RANGE_QUERY_SETTING = Setting.boolSetting( APPROXIMATE_POINT_RANGE_QUERY, false, Property.NodeScope ); - public static final String TERM_VERSION_PRECOMMIT_ENABLE = "opensearch.experimental.optimization.termversion.precommit.enabled"; + public static final String TERM_VERSION_PRECOMMIT_ENABLE = OS_EXPERIMENTAL_PREFIX + "optimization.termversion.precommit.enabled"; public static final Setting TERM_VERSION_PRECOMMIT_ENABLE_SETTING = Setting.boolSetting( TERM_VERSION_PRECOMMIT_ENABLE, false, Property.NodeScope ); - public static final String ARROW_STREAMS = "opensearch.experimental.feature.arrow.streams.enabled"; + public static final String ARROW_STREAMS = FEATURE_FLAG_PREFIX + "arrow.streams.enabled"; public static final Setting ARROW_STREAMS_SETTING = Setting.boolSetting(ARROW_STREAMS, false, Property.NodeScope); - private static final List> ALL_FEATURE_FLAG_SETTINGS = List.of( - REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING, - EXTENSIONS_SETTING, - TELEMETRY_SETTING, - DATETIME_FORMATTER_CACHING_SETTING, - WRITABLE_WARM_INDEX_SETTING, - STAR_TREE_INDEX_SETTING, - APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING, - READER_WRITER_SPLIT_EXPERIMENTAL_SETTING, - TERM_VERSION_PRECOMMIT_ENABLE_SETTING, - ARROW_STREAMS_SETTING - ); - /** - * Should store the settings from opensearch.yml. + * Underlying implementation for feature flags. + * All settable feature flags are tracked here in FeatureFlagsImpl.featureFlags. + * Contains all functionality across test and server use cases. */ - private static Settings settings; + static class FeatureFlagsImpl { + // Add an evergreen test feature flag and hide it in private scope + private static final String TEST_FLAG = "test.flag.enabled"; + private static final Setting TEST_FLAG_SETTING = Setting.boolSetting(TEST_FLAG, false, Property.NodeScope); + + private final ConcurrentHashMap, Boolean> featureFlags = new ConcurrentHashMap<>() { + { + put(TEST_FLAG_SETTING, TEST_FLAG_SETTING.get(Settings.EMPTY)); + put(REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING, REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)); + put(EXTENSIONS_SETTING, EXTENSIONS_SETTING.getDefault(Settings.EMPTY)); + put(TELEMETRY_SETTING, TELEMETRY_SETTING.getDefault(Settings.EMPTY)); + put(DATETIME_FORMATTER_CACHING_SETTING, DATETIME_FORMATTER_CACHING_SETTING.getDefault(Settings.EMPTY)); + put(WRITABLE_WARM_INDEX_SETTING, WRITABLE_WARM_INDEX_SETTING.getDefault(Settings.EMPTY)); + put(STAR_TREE_INDEX_SETTING, STAR_TREE_INDEX_SETTING.getDefault(Settings.EMPTY)); + put( + APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING, + APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING.getDefault(Settings.EMPTY) + ); + put(READER_WRITER_SPLIT_EXPERIMENTAL_SETTING, READER_WRITER_SPLIT_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)); + put(TERM_VERSION_PRECOMMIT_ENABLE_SETTING, TERM_VERSION_PRECOMMIT_ENABLE_SETTING.getDefault(Settings.EMPTY)); + put(ARROW_STREAMS_SETTING, ARROW_STREAMS_SETTING.getDefault(Settings.EMPTY)); + } + }; + + /** + * Initialize feature flags map from the following sources: + * (Each source overwrites previous feature flags) + * - Set from setting default + * - Set from JVM system property if flag exists + */ + FeatureFlagsImpl() { + initFromDefaults(); + initFromSysProperties(); + } + + /** + * Initialize feature flags map from the following sources: + * (Each source overwrites previous feature flags) + * - Set from setting default + * - Set from JVM system property if flag exists + * - Set from provided settings if flag exists + * @param openSearchSettings The settings stored in opensearch.yml. + */ + void initializeFeatureFlags(Settings openSearchSettings) { + initFromDefaults(); + initFromSysProperties(); + initFromSettings(openSearchSettings); + } + + /** + * Set all feature flags according to setting defaults. + * Overwrites existing entries in feature flags map. + * Skips flags which are locked according to TestUtils.FlagLock. + */ + private void initFromDefaults() { + for (Setting ff : featureFlags.keySet()) { + if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; + featureFlags.put(ff, ff.getDefault(Settings.EMPTY)); + } + } + + /** + * Update feature flags according to JVM system properties. + * Feature flags are true if system property is set as "true" (case-insensitive). Else feature set to false. + * Overwrites existing value if system property exists. + * Skips flags which are locked according to TestUtils.FlagLock. + */ + private void initFromSysProperties() { + for (Setting ff : featureFlags.keySet()) { + if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; + String prop = System.getProperty(ff.getKey()); + if (prop != null) { + featureFlags.put(ff, Boolean.valueOf(prop)); + } + } + } + + /** + * @param featureFlagName feature flag setting key + * @return true if feature enabled - else false + */ + boolean isEnabled(String featureFlagName) { + for (Setting ff : featureFlags.keySet()) { + if (ff.getKey().equals(featureFlagName)) return featureFlags.get(ff); + } + return false; + } - static { - Settings.Builder settingsBuilder = Settings.builder(); - for (Setting ffSetting : ALL_FEATURE_FLAG_SETTINGS) { - settingsBuilder = settingsBuilder.put(ffSetting.getKey(), ffSetting.getDefault(Settings.EMPTY)); + /** + * @param ff feature flag setting + * @return true if feature enabled - else false + */ + boolean isEnabled(Setting ff) { + if (!featureFlags.containsKey(ff)) return false; + return featureFlags.get(ff); + } + + /** + * @param featureFlagName feature flag key to set + * @param value value for flag + */ + void set(String featureFlagName, Boolean value) { + for (Setting ff : featureFlags.keySet()) { + if (ff.getKey().equals(featureFlagName)) featureFlags.put(ff, value); + } + } + + /** + * Update feature flags in ALL_FEATURE_FLAG_SETTINGS according to provided settings. + * Overwrites existing entries in feature flags map. + * Skips flags which are locked according to TestUtils.FlagLock. + * @param settings settings to update feature flags from + */ + private void initFromSettings(Settings settings) { + for (Setting ff : featureFlags.keySet()) { + if (settings.hasValue(ff.getKey())) { + if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; + featureFlags.put(ff, settings.getAsBoolean(ff.getKey(), ff.getDefault(settings))); + } + } } - settings = settingsBuilder.build(); } + private static final FeatureFlagsImpl featureFlagsImpl = new FeatureFlagsImpl(); + /** - * This method is responsible to map settings from opensearch.yml to local stored - * settings value. That is used for the existing isEnabled method. - * - * @param openSearchSettings The settings stored in opensearch.yml. + * Server module public API. */ public static void initializeFeatureFlags(Settings openSearchSettings) { - Settings.Builder settingsBuilder = Settings.builder(); - for (Setting ffSetting : ALL_FEATURE_FLAG_SETTINGS) { - settingsBuilder = settingsBuilder.put( - ffSetting.getKey(), - openSearchSettings.getAsBoolean(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings)) - ); - } - settings = settingsBuilder.build(); + featureFlagsImpl.initializeFeatureFlags(openSearchSettings); } - /** - * 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) { - if ("true".equalsIgnoreCase(System.getProperty(featureFlagName))) { - // TODO: Remove the if condition once FeatureFlags are only supported via opensearch.yml - return true; - } - return settings != null && settings.getAsBoolean(featureFlagName, false); + return featureFlagsImpl.isEnabled(featureFlagName); } public static boolean isEnabled(Setting featureFlag) { - if ("true".equalsIgnoreCase(System.getProperty(featureFlag.getKey()))) { - // TODO: Remove the if condition once FeatureFlags are only supported via opensearch.yml - return true; - } else if (settings != null) { - return featureFlag.get(settings); - } else { - return featureFlag.getDefault(Settings.EMPTY); + return featureFlagsImpl.isEnabled(featureFlag); + } + + /** + * Provides feature flag write access and synchronization for test use cases. + * To enable a feature flag for a single test case see @LockFeatureFlag annotation. + * For more fine grain control us TestUtils.with() or explicitly construct a new FlagLock(). + */ + public static class TestUtils { + /** + * Maintains an internal map of re-entrant locks corresponding to feature flags. + * Constructing a new FlagLock sets and locks the value of that feature flag. + * On unlock or close the flag is unlocked and reset to its previous value. + */ + public static class FlagLock implements AutoCloseable { + private static final Map flagLocks = new ConcurrentHashMap<>(); + private final String flagKey; + private final Boolean prev; + + public static boolean isLocked(String flagKey) { + if (!flagLocks.containsKey(flagKey)) return false; + return flagLocks.get(flagKey).isLocked(); + } + + public FlagLock(String flagKey) { + this(flagKey, true); + } + + public FlagLock(String flagKey, Boolean value) { + this.flagKey = flagKey; + this.prev = featureFlagsImpl.isEnabled(flagKey); + if (flagLocks.containsKey(flagKey) == false) { + flagLocks.put(flagKey, new ReentrantLock()); + } + flagLocks.get(flagKey).lock(); + featureFlagsImpl.set(flagKey, value); + } + + public void unlock() { + featureFlagsImpl.set(flagKey, prev); + flagLocks.get(flagKey).unlock(); + } + + @Override + public void close() { + featureFlagsImpl.set(flagKey, prev); + flagLocks.get(flagKey).unlock(); + } + } + + /** + * For critical sections run as lambdas which may throw exceptions. + */ + @FunctionalInterface + public interface ThrowingRunnable { + void run() throws Exception; + } + + /** + * Helper to synchronize a runnable test action on a feature flag to avoid race conditions when tests are run in parallel. + * Sets the feature flag back to its previous value after executing the test action. + * @param key feature flag setting key. + * @param action critical section to run while feature flag is set. + */ + public static void with(String key, ThrowingRunnable action) throws Exception { + try (FlagLock ignored = new FlagLock(key)) { + action.run(); + } + } + + public static void with(String key, Boolean value, ThrowingRunnable action) throws Exception { + try (FlagLock ignored = new FlagLock(key, value)) { + action.run(); + } } } } From 6ca4e92c5470cbfb46804ea0acd915889167653c Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 13:33:26 -0700 Subject: [PATCH 02/10] Add @LockFeatureFlag annotation - Add annotation in OpenSearchTestCase to enable and lock a flag for the duration of a single test case. Signed-off-by: Finn Carroll --- .../opensearch/test/OpenSearchTestCase.java | 62 +++++++++++++++++-- 1 file changed, 57 insertions(+), 5 deletions(-) diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java index 0bd5d8afda91e..b06947e70ba86 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java @@ -85,6 +85,7 @@ import org.opensearch.common.time.DateUtils; import org.opensearch.common.time.FormatNames; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.MockBigArrays; import org.opensearch.common.util.MockPageCacheRecycler; import org.opensearch.common.util.concurrent.ThreadContext; @@ -146,11 +147,18 @@ import org.junit.Rule; import org.junit.internal.AssumptionViolatedException; import org.junit.rules.RuleChain; +import org.junit.rules.TestRule; +import org.junit.runner.Description; +import org.junit.runners.model.Statement; import java.io.IOException; import java.io.InputStream; import java.io.PrintWriter; import java.io.StringWriter; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; import java.math.BigInteger; import java.net.InetAddress; import java.net.UnknownHostException; @@ -234,6 +242,51 @@ public abstract class OpenSearchTestCase extends LuceneTestCase { private static final Collection nettyLoggedLeaks = new ArrayList<>(); private HeaderWarningAppender headerWarningAppender; + /** + * Define LockFeatureFlag annotation for unit tests. + * Enables and locks a feature flag for the duration of the test case. + * This annotation guarantees the feature flag will be enabled for the duration of + * the test case. Feature flag is returned to previous value on test exit. + * Usage: + * @LockFeatureFlag("example.featureflag.setting.key.enabled") + * public void testFeatureFlagExample() { + * // Test flag dependant feature + * } + */ + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD }) + public @interface LockFeatureFlag { + String value(); + } + + public static class AnnotatedFeatureFlagRule implements TestRule { + /** + * If test is annotated with @LockFeatureFlag wrap the base Statement with the appropriate + * FeatureFlags.TestUtils.FlagLock to ensure test is synchronized on that flag. + * @param base test case to execute. + * @param description annotated test description. + */ + @Override + public Statement apply(Statement base, Description description) { + LockFeatureFlag annotation = description.getAnnotation(LockFeatureFlag.class); + if (annotation == null) { + return base; + } + String flagKey = annotation.value(); + return new Statement() { + @Override + public void evaluate() throws Throwable { + try (FeatureFlags.TestUtils.FlagLock ignored = new FeatureFlags.TestUtils.FlagLock(flagKey)) { + base.evaluate(); + } + } + }; + } + } + + @Rule + public AnnotatedFeatureFlagRule flagLockRule = new AnnotatedFeatureFlagRule(); + @AfterClass public static void resetPortCounter() { portGenerator.set(0); @@ -242,7 +295,6 @@ public static void resetPortCounter() { @Override public void tearDown() throws Exception { Schedulers.shutdownNow(); - FeatureFlagSetter.clear(); super.tearDown(); } @@ -1233,7 +1285,7 @@ public static boolean terminate(ThreadPool threadPool) { } /** - * Returns a {@link java.nio.file.Path} pointing to the class path relative resource given + * Returns a {@link Path} pointing to the class path relative resource given * as the first argument. In contrast to * getClass().getResource(...).getFile() this method will not * return URL encoded paths if the parent path contains spaces or other @@ -1395,7 +1447,7 @@ protected final BytesReference toShuffledXContent( boolean humanReadable, String... exceptFieldNames ) throws IOException { - BytesReference bytes = org.opensearch.core.xcontent.XContentHelper.toXContent(toXContent, mediaType, params, humanReadable); + BytesReference bytes = XContentHelper.toXContent(toXContent, mediaType, params, humanReadable); try (XContentParser parser = createParser(mediaType.xContent(), bytes)) { try (XContentBuilder builder = shuffleXContent(parser, rarely(), exceptFieldNames)) { return BytesReference.bytes(builder); @@ -1687,8 +1739,8 @@ protected static long spinForAtLeastNMilliseconds(final long ms) { protected IndexAnalyzers createDefaultIndexAnalyzers() { return new IndexAnalyzers( Collections.singletonMap("default", new NamedAnalyzer("default", AnalyzerScope.INDEX, new StandardAnalyzer())), - Collections.emptyMap(), - Collections.emptyMap() + emptyMap(), + emptyMap() ); } From 57115ef7fb2a4ff590590c24d623f2a8ddd68866 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 13:34:44 -0700 Subject: [PATCH 03/10] Update FeatureFlagTests - Add cases for public api. - Add cases for thread safe helpers @LockFeatureFlag FlagLock TestUtils.with Signed-off-by: Finn Carroll --- .../common/util/FeatureFlagTests.java | 170 ++++++++++++++---- 1 file changed, 140 insertions(+), 30 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index 6d9d1aad3c5d5..eae3c710d0efa 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -8,54 +8,164 @@ package org.opensearch.common.util; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.settings.Settings; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; -import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING; -import static org.opensearch.common.util.FeatureFlags.EXTENSIONS; +import static java.lang.Thread.sleep; +import static org.opensearch.common.util.FeatureFlags.FEATURE_FLAG_PREFIX; public class FeatureFlagTests extends OpenSearchTestCase { + // Evergreen test flag + private static final String TEST_FLAG = "test.flag.enabled"; - private final String FLAG_PREFIX = "opensearch.experimental.feature."; + public void testFeatureFlagsNotInitialized() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); + } + + public void testFeatureFlagsFromDefault() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); + } + + public void testFeatureFlagFromEmpty() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); + } + + public void testFeatureFlagFromSettings() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, true).build()); + assertTrue(testFlagsImpl.isEnabled(TEST_FLAG)); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); + } + + @SuppressForbidden(reason = "Testing system property functionality") + private void setSystemPropertyTrue(String key) { + System.setProperty(key, "true"); + } - public void testFeatureFlagSet() { - final String testFlag = FLAG_PREFIX + "testFlag"; - FeatureFlagSetter.set(testFlag); - assertNotNull(System.getProperty(testFlag)); - assertTrue(FeatureFlags.isEnabled(testFlag)); + @SuppressForbidden(reason = "Testing system property functionality") + private String getSystemProperty(String key) { + return System.getProperty(key); } - public void testMissingFeatureFlag() { - final String testFlag = FLAG_PREFIX + "testFlag"; - assertNull(System.getProperty(testFlag)); - assertFalse(FeatureFlags.isEnabled(testFlag)); + @SuppressForbidden(reason = "Testing system property functionality") + private void clearSystemProperty(String key) { + System.clearProperty(key); } public void testNonBooleanFeatureFlag() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); String javaVersionProperty = "java.version"; - assertNotNull(System.getProperty(javaVersionProperty)); - assertFalse(FeatureFlags.isEnabled(javaVersionProperty)); + assertNotNull(getSystemProperty(javaVersionProperty)); + assertFalse(testFlagsImpl.isEnabled(javaVersionProperty)); } - public void testBooleanFeatureFlagWithDefaultSetToFalse() { - final String testFlag = EXTENSIONS; - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); - assertNotNull(testFlag); - assertFalse(FeatureFlags.isEnabled(testFlag)); + public void testFeatureFlagFromSystemProperty() { + synchronized (TEST_FLAG) { // sync for sys property + setSystemPropertyTrue(TEST_FLAG); + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + assertTrue(testFlagsImpl.isEnabled(TEST_FLAG)); + clearSystemProperty(TEST_FLAG); + } + } + + @SuppressForbidden(reason = "Testing with system property") + public void testFeatureFlagSettingOverwritesSystemProperties() { + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + synchronized (TEST_FLAG) { // sync for sys property + setSystemPropertyTrue(TEST_FLAG); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertTrue(testFlagsImpl.isEnabled(TEST_FLAG)); + clearSystemProperty(TEST_FLAG); + } + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); + } + + @SuppressForbidden(reason = "Testing with system property") + public void testFeatureDoesNotExist() { + final String DNE_FF = FEATURE_FLAG_PREFIX + "doesntexist"; + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + assertFalse(testFlagsImpl.isEnabled(DNE_FF)); + setSystemPropertyTrue(DNE_FF); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertFalse(testFlagsImpl.isEnabled(DNE_FF)); + clearSystemProperty(DNE_FF); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(DNE_FF, true).build()); + assertFalse(testFlagsImpl.isEnabled(DNE_FF)); + } + + /** + * Test global feature flag instance. + */ + + public void testLockFeatureFlagWithFlagLock() { + try (FeatureFlags.TestUtils.FlagLock ignore = new FeatureFlags.TestUtils.FlagLock(TEST_FLAG)) { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked + } + } + + public void testLockFeatureFlagWithHelper() throws Exception { + FeatureFlags.TestUtils.with(TEST_FLAG, () -> { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked + }); + } + + @LockFeatureFlag(TEST_FLAG) + public void testLockFeatureFlagAnnotation() { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked + } + + // Attempt to set TEST_FLAG false every 2ms for a maximum of 2seconds. + private Thread attackTestFlag() { + Thread attackerThread = new Thread(() -> { + long endTime = System.currentTimeMillis() + 2000; + while (System.currentTimeMillis() < endTime) { + try { + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + sleep(2); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + break; + } + } + }); + + attackerThread.start(); + return attackerThread; } - public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToFalse() { - final String testFlag = DATETIME_FORMATTER_CACHING; - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); - assertFalse(FeatureFlags.isEnabled(testFlag)); + public void testAttackFlagLock() { + Thread attacker = attackTestFlag(); + try (FeatureFlags.TestUtils.FlagLock ignore = new FeatureFlags.TestUtils.FlagLock(TEST_FLAG)) { + for (int i = 0; i < 100; i++) { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + sleep(2); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + attacker.interrupt(); } - public void testInitializeFeatureFlagsWithExperimentalSettings() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(EXTENSIONS, true).build()); - assertTrue(FeatureFlags.isEnabled(EXTENSIONS)); - assertFalse(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING)); - // reset FeatureFlags to defaults - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + @LockFeatureFlag(TEST_FLAG) + public void testAttackAnnotationFlagLock() throws InterruptedException { + Thread attacker = attackTestFlag(); + for (int i = 0; i < 100; i++) { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + sleep(2); + } + attacker.interrupt(); } } From 5365a94fa7d5e176dab74712302f631ffadc69fe Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 13:35:18 -0700 Subject: [PATCH 04/10] Remove FeatureFlagSetter Signed-off-by: Finn Carroll --- .../opensearch/test/FeatureFlagSetter.java | 66 ------------------- 1 file changed, 66 deletions(-) delete mode 100644 test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java diff --git a/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java b/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java deleted file mode 100644 index f698cd03c464f..0000000000000 --- a/test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java +++ /dev/null @@ -1,66 +0,0 @@ -/* - * 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.test; - -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.SuppressForbidden; -import org.opensearch.common.util.concurrent.ConcurrentCollections; - -import java.security.AccessController; -import java.security.PrivilegedAction; -import java.util.Set; - -/** - * Helper class that wraps the lifecycle of setting and finally clearing of - * a {@link org.opensearch.common.util.FeatureFlags} string. - */ -public class FeatureFlagSetter { - - private static FeatureFlagSetter INSTANCE = null; - - private static synchronized FeatureFlagSetter getInstance() { - if (INSTANCE == null) { - INSTANCE = new FeatureFlagSetter(); - } - return INSTANCE; - } - - public static synchronized void set(String flag) { - getInstance().setFlag(flag); - } - - public static synchronized void clear() { - if (INSTANCE != null) { - INSTANCE.clearAll(); - INSTANCE = null; - } - } - - private static final Logger LOGGER = LogManager.getLogger(FeatureFlagSetter.class); - private final Set flags = ConcurrentCollections.newConcurrentSet(); - - @SuppressWarnings("removal") - @SuppressForbidden(reason = "Enables setting of feature flags") - private void setFlag(String flag) { - flags.add(flag); - AccessController.doPrivileged((PrivilegedAction) () -> System.setProperty(flag, "true")); - LOGGER.info("set feature_flag={}", flag); - } - - @SuppressWarnings("removal") - @SuppressForbidden(reason = "Clears the set feature flags") - private void clearAll() { - for (String flag : flags) { - AccessController.doPrivileged((PrivilegedAction) () -> System.clearProperty(flag)); - } - LOGGER.info("unset feature_flags={}", flags); - flags.clear(); - } -} From 558bffef080fdd9d72e9a944dff42b5c594c02b6 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 13:36:51 -0700 Subject: [PATCH 05/10] Find and replace feature flag test case usages with appropriate helper Replace all usages of `FeatureFlagSetter` in tests. Replace all usages of JVM system properties for feature flags in tests. Replace all usages of `initializeFeatureFlags` with `TestUtils.set` in tests. Signed-off-by: Finn Carroll --- .../mapper/ScaledFloatFieldMapperTests.java | 14 +--- .../arrow/flight/ArrowFlightServerIT.java | 11 +-- .../arrow/flight/FlightStreamPluginTests.java | 10 +-- .../bootstrap/FlightClientManagerTests.java | 5 +- .../flight/bootstrap/FlightServiceTests.java | 5 +- .../shrink/TransportResizeActionTests.java | 4 +- .../SystemTemplatesServiceTests.java | 7 +- .../coordination/JoinTaskExecutorTests.java | 13 ++-- .../MetadataCreateIndexServiceTests.java | 71 +++++++++---------- .../MetadataIndexTemplateServiceTests.java | 17 ++--- .../allocation/FailedShardsRoutingTests.java | 3 +- .../ShardsTieringAllocationTests.java | 14 ++-- .../common/settings/SettingsModuleTests.java | 7 +- .../extensions/ExtensionsManagerTests.java | 6 +- .../RemoteClusterStateServiceTests.java | 5 +- .../opensearch/index/IndexSettingsTests.java | 5 +- .../AbstractStarTreeDVFormatTests.java | 7 +- .../index/engine/ReadOnlyEngineTests.java | 5 +- .../index/mapper/ObjectMapperTests.java | 7 +- .../index/mapper/StarTreeMapperTests.java | 6 +- .../opensearch/index/store/StoreTests.java | 5 +- .../java/org/opensearch/node/NodeTests.java | 5 +- .../opensearch/search/SearchModuleTests.java | 3 - .../search/SearchServiceStarTreeTests.java | 10 +-- .../DateHistogramAggregatorTests.java | 7 +- .../startree/KeywordTermsAggregatorTests.java | 7 +- .../startree/MetricAggregatorTests.java | 7 +- .../startree/NumericTermsAggregatorTests.java | 7 +- .../startree/StarTreeFilterTests.java | 7 +- 29 files changed, 120 insertions(+), 160 deletions(-) diff --git a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java index d1af54452bde9..7dcfe1848511e 100644 --- a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java +++ b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java @@ -36,15 +36,12 @@ import org.apache.lucene.index.IndexableField; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.compositeindex.datacube.startree.StarTreeIndexSettings; import org.opensearch.plugins.Plugin; -import org.junit.AfterClass; -import org.junit.BeforeClass; import java.io.IOException; import java.util.Arrays; @@ -98,16 +95,7 @@ public void testExistsQueryDocValuesDisabled() throws IOException { assertParseMinimalWarnings(); } - @BeforeClass - public static void createMapper() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(STAR_TREE_INDEX, "true").build()); - } - - @AfterClass - public static void clearMapper() { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); - } - + @LockFeatureFlag(STAR_TREE_INDEX) public void testScaledFloatWithStarTree() throws Exception { double scalingFactorField1 = randomDouble() * 100; diff --git a/plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/ArrowFlightServerIT.java b/plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/ArrowFlightServerIT.java index bcad335c7a917..54b47329dab7f 100644 --- a/plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/ArrowFlightServerIT.java +++ b/plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/ArrowFlightServerIT.java @@ -14,26 +14,20 @@ import org.opensearch.arrow.flight.bootstrap.FlightService; import org.opensearch.arrow.flight.bootstrap.FlightStreamPlugin; import org.opensearch.cluster.node.DiscoveryNode; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.plugins.Plugin; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchIntegTestCase; -import org.junit.BeforeClass; import java.util.Collection; import java.util.Collections; import java.util.concurrent.TimeUnit; +import static org.opensearch.common.util.FeatureFlags.ARROW_STREAMS; + @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE, numDataNodes = 5) public class ArrowFlightServerIT extends OpenSearchIntegTestCase { private FlightClientManager flightClientManager; - @BeforeClass - public static void setupFeatureFlags() { - FeatureFlagSetter.set(FeatureFlags.ARROW_STREAMS_SETTING.getKey()); - } - @Override protected Collection> nodePlugins() { return Collections.singleton(FlightStreamPlugin.class); @@ -48,6 +42,7 @@ public void setUp() throws Exception { flightClientManager = flightService.getFlightClientManager(); } + @LockFeatureFlag(ARROW_STREAMS) public void testArrowFlightEndpoint() throws Exception { for (DiscoveryNode node : getClusterState().nodes()) { try (FlightClient flightClient = flightClientManager.getFlightClient(node.getId()).get()) { diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/FlightStreamPluginTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/FlightStreamPluginTests.java index 6f93d792f9db4..2573f0032f45b 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/FlightStreamPluginTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/FlightStreamPluginTests.java @@ -19,9 +19,7 @@ import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.plugins.SecureTransportSettingsProvider; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.threadpool.ExecutorBuilder; import org.opensearch.threadpool.ThreadPool; @@ -31,19 +29,18 @@ import java.util.List; import java.util.function.Supplier; -import static org.opensearch.common.util.FeatureFlags.ARROW_STREAMS_SETTING; +import static org.opensearch.common.util.FeatureFlags.ARROW_STREAMS; import static org.opensearch.plugins.NetworkPlugin.AuxTransport.AUX_TRANSPORT_TYPES_KEY; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; public class FlightStreamPluginTests extends OpenSearchTestCase { - private Settings settings; + private final Settings settings = Settings.EMPTY; private ClusterService clusterService; @Override public void setUp() throws Exception { super.setUp(); - settings = Settings.builder().put(ARROW_STREAMS_SETTING.getKey(), true).build(); clusterService = mock(ClusterService.class); ClusterState clusterState = mock(ClusterState.class); DiscoveryNodes nodes = mock(DiscoveryNodes.class); @@ -52,9 +49,8 @@ public void setUp() throws Exception { when(nodes.getLocalNodeId()).thenReturn("test-node"); } + @LockFeatureFlag(ARROW_STREAMS) public void testPluginEnabled() throws IOException { - FeatureFlags.initializeFeatureFlags(settings); - FeatureFlagSetter.set(ARROW_STREAMS_SETTING.getKey()); FlightStreamPlugin plugin = new FlightStreamPlugin(settings); Collection components = plugin.createComponents( null, diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java index acc32d6b32f4c..f8b04197805d1 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java @@ -24,11 +24,9 @@ import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.transport.BoundTransportAddress; import org.opensearch.core.common.transport.TransportAddress; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.Client; @@ -55,6 +53,7 @@ import io.netty.util.NettyRuntime; import static org.opensearch.arrow.flight.bootstrap.FlightClientManager.LOCATION_TIMEOUT_MS; +import static org.opensearch.common.util.FeatureFlags.ARROW_STREAMS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; @@ -84,12 +83,12 @@ public static void setupClass() throws Exception { executorService = ServerConfig.createELG("test-grpc-worker", NettyRuntime.availableProcessors() * 2); } + @LockFeatureFlag(ARROW_STREAMS) @Override public void setUp() throws Exception { super.setUp(); locationUpdaterExecutor = Executors.newScheduledThreadPool(1); - FeatureFlagSetter.set(FeatureFlags.ARROW_STREAMS_SETTING.getKey()); clusterService = mock(ClusterService.class); client = mock(Client.class); state = getDefaultState(); diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java index fa20535384557..e85d5a3893e45 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java @@ -19,7 +19,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.transport.TransportAddress; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.Client; @@ -32,10 +31,12 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicInteger; +import static org.opensearch.common.util.FeatureFlags.ARROW_STREAMS; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; public class FlightServiceTests extends OpenSearchTestCase { + FeatureFlags.TestUtils.FlagLock ffLock = null; private Settings settings; private ClusterService clusterService; @@ -47,7 +48,7 @@ public class FlightServiceTests extends OpenSearchTestCase { @Override public void setUp() throws Exception { super.setUp(); - FeatureFlagSetter.set(FeatureFlags.ARROW_STREAMS_SETTING.getKey()); + ffLock = new FeatureFlags.TestUtils.FlagLock(ARROW_STREAMS); int availablePort = getBasePort(9500) + port.addAndGet(1); settings = Settings.EMPTY; localNode = createNode(availablePort); diff --git a/server/src/test/java/org/opensearch/action/admin/indices/shrink/TransportResizeActionTests.java b/server/src/test/java/org/opensearch/action/admin/indices/shrink/TransportResizeActionTests.java index 5bab2ceca0988..18069ede796af 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/shrink/TransportResizeActionTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/shrink/TransportResizeActionTests.java @@ -53,7 +53,6 @@ import org.opensearch.cluster.routing.allocation.decider.MaxRetryAllocationDecider; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.shard.DocsStats; import org.opensearch.index.store.StoreStats; @@ -602,11 +601,10 @@ public void testIndexBlocks() { assertEquals(request.waitForActiveShards(), activeShardCount); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testResizeFailuresDuringMigration() { // We will keep all other settings correct for resize request, // So we only need to test for the failures due to cluster setting validation while migration - final Settings directionEnabledNodeSettings = Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build(); - FeatureFlags.initializeFeatureFlags(directionEnabledNodeSettings); boolean isRemoteStoreEnabled = randomBoolean(); CompatibilityMode compatibilityMode = randomFrom(CompatibilityMode.values()); RemoteStoreNodeService.Direction migrationDirection = randomFrom(RemoteStoreNodeService.Direction.values()); diff --git a/server/src/test/java/org/opensearch/cluster/applicationtemplates/SystemTemplatesServiceTests.java b/server/src/test/java/org/opensearch/cluster/applicationtemplates/SystemTemplatesServiceTests.java index affb017264fdf..6619f2587d188 100644 --- a/server/src/test/java/org/opensearch/cluster/applicationtemplates/SystemTemplatesServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/applicationtemplates/SystemTemplatesServiceTests.java @@ -11,7 +11,6 @@ import org.opensearch.cluster.service.applicationtemplates.TestSystemTemplatesRepositoryPlugin; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.threadpool.ThreadPool; @@ -23,12 +22,14 @@ import org.mockito.Mockito; import static org.opensearch.common.settings.ClusterSettings.BUILT_IN_CLUSTER_SETTINGS; +import static org.opensearch.common.util.FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES; import static org.mockito.Mockito.when; public class SystemTemplatesServiceTests extends OpenSearchTestCase { private SystemTemplatesService systemTemplatesService; + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testSystemTemplatesLoaded() throws IOException { setupService(true); @@ -43,6 +44,7 @@ public void testSystemTemplatesLoaded() throws IOException { } } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testSystemTemplatesVerifyAndLoad() throws IOException { setupService(false); @@ -61,6 +63,7 @@ public void testSystemTemplatesVerifyAndLoad() throws IOException { assertEquals(stats.getFailedLoadingRepositories(), 0L); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testSystemTemplatesVerifyWithFailingRepository() throws IOException { setupService(true); @@ -77,8 +80,6 @@ public void testSystemTemplatesVerifyWithFailingRepository() throws IOException } private void setupService(boolean errorFromMockPlugin) throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); - ThreadPool mockPool = Mockito.mock(ThreadPool.class); when(mockPool.generic()).thenReturn(OpenSearchExecutors.newDirectExecutorService()); diff --git a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java index 9b91e4d507d57..ab455a1fbb4e7 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java @@ -52,7 +52,6 @@ import org.opensearch.common.SetOnce; import org.opensearch.common.UUIDs; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.node.remotestore.RemoteStoreNodeService; import org.opensearch.repositories.RepositoriesService; import org.opensearch.repositories.RepositoryMissingException; @@ -958,6 +957,7 @@ public void testUpdatesClusterStateWithMultiNodeClusterAndSameRepository() throw validateRepositoryMetadata(result.resultingState, clusterManagerNode, 2); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testUpdatesRepoRemoteNodeJoinPublicationCluster() throws Exception { final AllocationService allocationService = mock(AllocationService.class); when(allocationService.adaptAutoExpandReplicas(any())).then(invocationOnMock -> invocationOnMock.getArguments()[0]); @@ -1005,8 +1005,6 @@ public void testUpdatesRepoRemoteNodeJoinPublicationCluster() throws Exception { .put(MIGRATION_DIRECTION_SETTING.getKey(), RemoteStoreNodeService.Direction.REMOTE_STORE) .put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), "mixed") .build(); - final Settings nodeSettings = Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build(); - FeatureFlags.initializeFeatureFlags(nodeSettings); Metadata metadata = Metadata.builder().persistentSettings(settings).build(); ClusterState currentState = ClusterState.builder(result.resultingState).metadata(metadata).build(); @@ -1029,6 +1027,7 @@ public void testUpdatesRepoRemoteNodeJoinPublicationCluster() throws Exception { validateRepositoriesMetadata(resultAfterRemoteNodeJoin.resultingState, remoteStoreNode, clusterManagerNode); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testUpdatesRepoPublicationNodeJoinRemoteCluster() throws Exception { final AllocationService allocationService = mock(AllocationService.class); when(allocationService.adaptAutoExpandReplicas(any())).then(invocationOnMock -> invocationOnMock.getArguments()[0]); @@ -1071,8 +1070,6 @@ public void testUpdatesRepoPublicationNodeJoinRemoteCluster() throws Exception { .put(MIGRATION_DIRECTION_SETTING.getKey(), RemoteStoreNodeService.Direction.REMOTE_STORE) .put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), "mixed") .build(); - final Settings nodeSettings = Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build(); - FeatureFlags.initializeFeatureFlags(nodeSettings); Metadata metadata = Metadata.builder().persistentSettings(settings).build(); ClusterState currentState = ClusterState.builder(result.resultingState).metadata(metadata).build(); @@ -1309,6 +1306,7 @@ public void testRemoteRoutingTableNodeJoinNodeWithRemoteAndRoutingRepoDifference JoinTaskExecutor.ensureNodesCompatibility(joiningNode, currentState.getNodes(), currentState.metadata()); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testRemoteRoutingTableNodeJoinNodeWithRemoteAndRoutingRepoDifferenceMixedMode() { Map attr = remoteStoreNodeAttributes(SEGMENT_REPO, TRANSLOG_REPO); attr.putAll(remoteRoutingTableAttributes(ROUTING_TABLE_REPO)); @@ -1332,8 +1330,6 @@ public void testRemoteRoutingTableNodeJoinNodeWithRemoteAndRoutingRepoDifference .put(MIGRATION_DIRECTION_SETTING.getKey(), RemoteStoreNodeService.Direction.REMOTE_STORE) .put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), "mixed") .build(); - final Settings nodeSettings = Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build(); - FeatureFlags.initializeFeatureFlags(nodeSettings); Metadata metadata = Metadata.builder().persistentSettings(settings).build(); ClusterState currentState = ClusterState.builder(ClusterName.DEFAULT) .nodes(DiscoveryNodes.builder().add(existingNode2).add(existingNode).localNodeId(existingNode.getId()).build()) @@ -1344,6 +1340,7 @@ public void testRemoteRoutingTableNodeJoinNodeWithRemoteAndRoutingRepoDifference JoinTaskExecutor.ensureNodesCompatibility(joiningNode, currentState.getNodes(), currentState.metadata()); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testJoinRemoteStoreClusterWithRemotePublicationNodeInMixedMode() { final DiscoveryNode remoteStoreNode = new DiscoveryNode( UUIDs.base64UUID(), @@ -1365,8 +1362,6 @@ public void testJoinRemoteStoreClusterWithRemotePublicationNodeInMixedMode() { .put(MIGRATION_DIRECTION_SETTING.getKey(), RemoteStoreNodeService.Direction.REMOTE_STORE) .put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), "mixed") .build(); - final Settings nodeSettings = Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build(); - FeatureFlags.initializeFeatureFlags(nodeSettings); Metadata metadata = Metadata.builder().persistentSettings(settings).build(); ClusterState currentState = ClusterState.builder(ClusterName.DEFAULT) .nodes(DiscoveryNodes.builder().add(remoteStoreNode).add(nonRemoteStoreNode).localNodeId(remoteStoreNode.getId()).build()) diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java index dfe3928ac37f3..03237ba81f05e 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -156,6 +156,7 @@ import static org.opensearch.cluster.metadata.MetadataCreateIndexService.parseV1Mappings; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.resolveAndValidateAliases; import static org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider.INDEX_TOTAL_PRIMARY_SHARDS_PER_NODE_SETTING; +import static org.opensearch.common.util.FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES; import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE_MIGRATION_EXPERIMENTAL; import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; import static org.opensearch.index.IndexSettings.INDEX_MERGE_POLICY; @@ -247,8 +248,6 @@ public void setupCreateIndexRequestAndAliasValidator() { @After public void tearDown() throws Exception { super.tearDown(); - // clear any FeatureFlags needed for individual tests - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); clusterSettings = null; } @@ -1600,9 +1599,8 @@ public void testRemoteStoreOverrideTranslogRepoIndexSettings() { })); } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build()); - // non-remote cluster manager node DiscoveryNode nonRemoteClusterManagerNode = new DiscoveryNode(UUIDs.base64UUID(), buildNewFakeTransportAddress(), Version.CURRENT); @@ -2314,40 +2312,41 @@ public void testIndexCreationWithIndexStoreTypeRemoteStoreThrowsException() { public void testCreateIndexWithContextDisabled() throws Exception { // Explicitly disable the FF - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, false).build()); - request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test").context(new Context(randomAlphaOfLength(5))); - withTemporaryClusterService((clusterService, threadPool) -> { - MetadataCreateIndexService checkerService = new MetadataCreateIndexService( - Settings.EMPTY, - clusterService, - indicesServices, - null, - null, - createTestShardLimitService(randomIntBetween(1, 1000), false, clusterService), - mock(Environment.class), - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - threadPool, - null, - new SystemIndices(Collections.emptyMap()), - false, - new AwarenessReplicaBalance(Settings.EMPTY, clusterService.getClusterSettings()), - DefaultRemoteStoreSettings.INSTANCE, - repositoriesServiceSupplier - ); - CountDownLatch counter = new CountDownLatch(1); - InvalidIndexContextException exception = expectThrows( - InvalidIndexContextException.class, - () -> checkerService.validateContext(request) - ); - assertTrue( - "Invalid exception message." + exception.getMessage(), - exception.getMessage().contains("index specifies a context which cannot be used without enabling") - ); + FeatureFlags.TestUtils.with(APPLICATION_BASED_CONFIGURATION_TEMPLATES, false, () -> { + request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test").context(new Context(randomAlphaOfLength(5))); + withTemporaryClusterService((clusterService, threadPool) -> { + MetadataCreateIndexService checkerService = new MetadataCreateIndexService( + Settings.EMPTY, + clusterService, + indicesServices, + null, + null, + createTestShardLimitService(randomIntBetween(1, 1000), false, clusterService), + mock(Environment.class), + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, + threadPool, + null, + new SystemIndices(Collections.emptyMap()), + false, + new AwarenessReplicaBalance(Settings.EMPTY, clusterService.getClusterSettings()), + DefaultRemoteStoreSettings.INSTANCE, + repositoriesServiceSupplier + ); + CountDownLatch counter = new CountDownLatch(1); + InvalidIndexContextException exception = expectThrows( + InvalidIndexContextException.class, + () -> checkerService.validateContext(request) + ); + assertTrue( + "Invalid exception message." + exception.getMessage(), + exception.getMessage().contains("index specifies a context which cannot be used without enabling") + ); + }); }); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testCreateIndexWithContextAbsent() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test").context(new Context(randomAlphaOfLength(5))); withTemporaryClusterService((clusterService, threadPool) -> { MetadataCreateIndexService checkerService = new MetadataCreateIndexService( @@ -2379,8 +2378,8 @@ public void testCreateIndexWithContextAbsent() throws Exception { }); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testApplyContext() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test").context(new Context(randomAlphaOfLength(5))); final Map mappings = new HashMap<>(); @@ -2476,8 +2475,8 @@ public void testApplyContext() throws IOException { }); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testApplyContextWithSettingsOverlap() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test").context(new Context(randomAlphaOfLength(5))); Settings.Builder settingsBuilder = Settings.builder().put(INDEX_REFRESH_INTERVAL_SETTING.getKey(), "30s"); String templateContent = "{\n" diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index 795d1713772c2..37ad8bdcaea67 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -47,7 +47,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.XContentFactory; @@ -95,6 +94,7 @@ import static org.opensearch.cluster.applicationtemplates.SystemTemplateMetadata.fromComponentTemplateInfo; import static org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider.INDEX_TOTAL_PRIMARY_SHARDS_PER_NODE_SETTING; import static org.opensearch.common.settings.Settings.builder; +import static org.opensearch.common.util.FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES; import static org.opensearch.common.util.concurrent.ThreadContext.ACTION_ORIGIN_TRANSIENT_NAME; import static org.opensearch.env.Environment.PATH_HOME_SETTING; import static org.opensearch.index.mapper.DataStreamFieldMapper.Defaults.TIMESTAMP_FIELD; @@ -769,8 +769,8 @@ public void onFailure(Exception e) { ); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testPutGlobalV2TemplateWhichProvidesContextNotPresentInState() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); ComposableIndexTemplate globalIndexTemplate = new ComposableIndexTemplate( List.of("*"), @@ -809,8 +809,8 @@ public void onFailure(Exception e) { ); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testPutGlobalV2TemplateWhichProvidesContextWithNonExistingVersion() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); Function templateApplier = codec -> new Template( @@ -893,8 +893,8 @@ public void onFailure(Exception e) { ); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testPutGlobalV2TemplateWhichProvidesContextInComposedOfSection() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); Function templateApplier = codec -> new Template( @@ -980,8 +980,8 @@ public void testPutGlobalV2TemplateWhichProvidesContextWithLatestVersion() throw verifyTemplateCreationUsingContext("_latest"); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testModifySystemTemplateViaUnknownSource() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); Function templateApplier = codec -> new Template( @@ -2629,8 +2629,8 @@ public static void assertTemplatesEqual(ComposableIndexTemplate actual, Composab } } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) private String verifyTemplateCreationUsingContext(String contextVersion) throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, true).build()); MetadataIndexTemplateService metadataIndexTemplateService = getMetadataIndexTemplateService(); Function templateApplier = codec -> new Template( @@ -2751,9 +2751,6 @@ protected boolean resetNodeAfterTest() { @Override protected Settings featureFlagSettings() { - return Settings.builder() - .put(super.featureFlagSettings()) - .put(FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES, false) - .build(); + return Settings.builder().put(super.featureFlagSettings()).put(APPLICATION_BASED_CONFIGURATION_TEMPLATES, false).build(); } } diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/FailedShardsRoutingTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/FailedShardsRoutingTests.java index f8e1c609e6ee8..d2b77a013a483 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/FailedShardsRoutingTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/FailedShardsRoutingTests.java @@ -50,7 +50,6 @@ import org.opensearch.cluster.routing.allocation.decider.ClusterRebalanceAllocationDecider; import org.opensearch.common.UUIDs; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.index.shard.ShardId; import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.node.remotestore.RemoteStoreNodeService; @@ -825,8 +824,8 @@ private void testReplicaIsPromoted(boolean isSegmentReplicationEnabled) { } } + @LockFeatureFlag(REMOTE_STORE_MIGRATION_EXPERIMENTAL) public void testPreferReplicaOnRemoteNodeForPrimaryPromotion() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build()); AllocationService allocation = createAllocationService(Settings.builder().build()); // segment replication enabled diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/ShardsTieringAllocationTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/ShardsTieringAllocationTests.java index 765d88f7af360..a1d5cb3932aa7 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/ShardsTieringAllocationTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/ShardsTieringAllocationTests.java @@ -15,23 +15,17 @@ import org.opensearch.cluster.routing.RoutingNodes; import org.opensearch.cluster.routing.RoutingPool; import org.opensearch.cluster.routing.ShardRouting; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexModule; -import org.opensearch.test.FeatureFlagSetter; -import org.junit.Before; import static org.opensearch.cluster.routing.RoutingPool.LOCAL_ONLY; import static org.opensearch.cluster.routing.RoutingPool.REMOTE_CAPABLE; import static org.opensearch.cluster.routing.RoutingPool.getIndexPool; +import static org.opensearch.common.util.FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG; import static org.opensearch.index.IndexModule.INDEX_STORE_LOCALITY_SETTING; public class ShardsTieringAllocationTests extends TieringAllocationBaseTestCase { - @Before - public void setup() { - FeatureFlagSetter.set(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG); - } - + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) public void testShardsInLocalPool() { int localOnlyNodes = 5; int remoteCapableNodes = 3; @@ -52,6 +46,7 @@ public void testShardsInLocalPool() { } } + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) public void testShardsInRemotePool() { int localOnlyNodes = 7; int remoteCapableNodes = 3; @@ -72,6 +67,7 @@ public void testShardsInRemotePool() { } } + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) public void testShardsWithTiering() { int localOnlyNodes = 15; int remoteCapableNodes = 13; @@ -104,6 +100,7 @@ public void testShardsWithTiering() { } } + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) public void testShardPoolForPartialIndices() { String index = "test-index"; IndexMetadata indexMetadata = IndexMetadata.builder(index) @@ -118,6 +115,7 @@ public void testShardPoolForPartialIndices() { assertEquals(REMOTE_CAPABLE, indexPool); } + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) public void testShardPoolForFullIndices() { String index = "test-index"; IndexMetadata indexMetadata = IndexMetadata.builder(index) diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java index d504c3af90679..0be28f580d73c 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java @@ -34,15 +34,14 @@ import org.opensearch.common.inject.ModuleTestCase; import org.opensearch.common.settings.Setting.Property; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexSettings; import org.opensearch.search.SearchService; -import org.opensearch.test.FeatureFlagSetter; import org.hamcrest.Matchers; import java.util.Arrays; import static java.util.Collections.emptySet; +import static org.opensearch.common.util.FeatureFlags.EXTENSIONS; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -242,8 +241,8 @@ public void testOldMaxClauseCountSetting() { ); } + @LockFeatureFlag(EXTENSIONS) public void testDynamicNodeSettingsRegistration() { - FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); assertNotNull(module.getClusterSettings().get("some.custom.setting")); @@ -263,8 +262,8 @@ public void testDynamicNodeSettingsRegistration() { ); } + @LockFeatureFlag(EXTENSIONS) public void testDynamicIndexSettingsRegistration() { - FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); Settings settings = Settings.builder().put("some.custom.setting", "2.0").build(); SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope)); assertNotNull(module.getClusterSettings().get("some.custom.setting")); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 8b0a455353c5f..7d2625d218d9e 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -46,7 +46,6 @@ import org.opensearch.plugins.ExtensionAwarePlugin; import org.opensearch.rest.RestController; import org.opensearch.telemetry.tracing.noop.NoopTracer; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.MockLogAppender; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.client.NoOpNodeClient; @@ -74,6 +73,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; +import static org.opensearch.common.util.FeatureFlags.EXTENSIONS; import static org.opensearch.test.ClusterServiceUtils.createClusterService; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -86,6 +86,7 @@ import static org.mockito.Mockito.when; public class ExtensionsManagerTests extends OpenSearchTestCase { + private static FeatureFlags.TestUtils.FlagLock ffLock = null; private TransportService transportService; private ActionModule actionModule; private DynamicActionRegistry dynamicActionRegistry; @@ -108,7 +109,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { @Before public void setup() throws Exception { - FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); + ffLock = new FeatureFlags.TestUtils.FlagLock(EXTENSIONS); Settings settings = Settings.builder().put("cluster.name", "test").build(); transport = new MockNioTransport( settings, @@ -179,6 +180,7 @@ public void tearDown() throws Exception { super.tearDown(); transportService.close(); client.close(); + ffLock.close(); ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); } diff --git a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java index e3684178a18ea..4fffab79e036b 100644 --- a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java +++ b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java @@ -45,7 +45,6 @@ import org.opensearch.common.remote.AbstractClusterMetadataWriteableBlobEntity; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; @@ -2751,6 +2750,7 @@ public void testRemoteRoutingTableNotInitializedWhenDisabled() { } } + @LockFeatureFlag(REMOTE_PUBLICATION_SETTING_KEY) public void testRemoteRoutingTableInitializedWhenEnabled() { Settings newSettings = Settings.builder() .put("node.attr." + REMOTE_STORE_ROUTING_TABLE_REPOSITORY_NAME_ATTRIBUTE_KEY, "routing_repository") @@ -2759,9 +2759,6 @@ public void testRemoteRoutingTableInitializedWhenEnabled() { .build(); clusterSettings.applySettings(newSettings); - Settings nodeSettings = Settings.builder().put(REMOTE_PUBLICATION_SETTING_KEY, "true").build(); - FeatureFlags.initializeFeatureFlags(nodeSettings); - remoteClusterStateService = new RemoteClusterStateService( "test-node-id", repositoriesServiceSupplier, diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index bc505daa607c1..7ea9dd336ccb8 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -42,12 +42,10 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.translog.Translog; import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.search.pipeline.SearchPipelineService; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; @@ -60,6 +58,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.core.StringContains.containsString; @@ -997,9 +996,9 @@ public void testUpdateRemoteTranslogBufferInterval() { ); } + @LockFeatureFlag(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testExtendedCompatibilityVersionForRemoteSnapshot() throws Exception { - FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); IndexMetadata metadata = newIndexMeta( "index", Settings.builder() diff --git a/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java b/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java index 18a7bb03b0a59..f06f747a72880 100644 --- a/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java +++ b/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java @@ -51,6 +51,7 @@ */ @LuceneTestCase.SuppressSysoutChecks(bugUrl = "we log a lot on purpose") public abstract class AbstractStarTreeDVFormatTests extends BaseDocValuesFormatTestCase { + private static FeatureFlags.TestUtils.FlagLock ffLock = null; MapperService mapperService = null; StarTreeFieldConfiguration.StarTreeBuildMode buildMode; @@ -67,13 +68,13 @@ public static Collection parameters() { } @BeforeClass - public static void createMapper() throws Exception { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(STAR_TREE_INDEX, "true").build()); + public static void createMapper() { + ffLock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @AfterClass public static void clearMapper() { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + ffLock.close(); } @After diff --git a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java index 288822ab1589f..6979407690749 100644 --- a/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/ReadOnlyEngineTests.java @@ -41,7 +41,6 @@ import org.opensearch.common.lucene.LuceneTests; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.index.IndexModule; @@ -51,7 +50,6 @@ import org.opensearch.index.seqno.SequenceNumbers; import org.opensearch.index.store.Store; import org.opensearch.index.translog.TranslogStats; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; import java.io.IOException; @@ -62,6 +60,7 @@ import java.util.function.Function; import static org.opensearch.common.lucene.index.OpenSearchDirectoryReader.getOpenSearchDirectoryReader; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.instanceOf; @@ -240,13 +239,13 @@ public void testReadOnly() throws IOException { } } + @LockFeatureFlag(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) public void testReadOldIndices() throws Exception { IOUtils.close(engine, store); // The index has one document in it, so the checkpoint cannot be NO_OPS_PERFORMED final AtomicLong globalCheckpoint = new AtomicLong(0); Path tmp = createTempDir(); TestUtil.unzip(getClass().getResourceAsStream(LuceneTests.OLDER_VERSION_INDEX_ZIP_RELATIVE_PATH), tmp); - FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( "index", Settings.builder() diff --git a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java index 45483ef51a5f9..b5f3e8fb8331b 100644 --- a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java @@ -35,7 +35,6 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.compress.CompressedXContent; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.unit.ByteSizeUnit; @@ -499,6 +498,7 @@ public void testDerivedFields() throws Exception { assertEquals("date", mapper.typeName()); } + @LockFeatureFlag(STAR_TREE_INDEX) public void testCompositeFields() throws Exception { String mapping = XContentFactory.jsonBuilder() .startObject() @@ -556,9 +556,6 @@ public void testCompositeFields() throws Exception { ex.getMessage() ); - final Settings starTreeEnabledSettings = Settings.builder().put(STAR_TREE_INDEX, "true").build(); - FeatureFlags.initializeFeatureFlags(starTreeEnabledSettings); - DocumentMapper documentMapper = createIndex("test", settings).mapperService() .documentMapperParser() .parse("tweet", new CompressedXContent(mapping)); @@ -571,8 +568,6 @@ public void testCompositeFields() throws Exception { mapper = documentMapper.root().getMapper("@timestamp"); assertNotNull(mapper); assertEquals("date", mapper.typeName()); - - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); } public void testNestedIsParent() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java index 435621548942b..140b2e7694546 100644 --- a/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java @@ -43,6 +43,7 @@ import java.util.List; import java.util.Set; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; import static org.opensearch.index.IndexSettings.INDEX_TRANSLOG_FLUSH_THRESHOLD_SIZE_SETTING; import static org.opensearch.index.compositeindex.CompositeIndexSettings.COMPOSITE_INDEX_MAX_TRANSLOG_FLUSH_THRESHOLD_SIZE_SETTING; @@ -53,15 +54,16 @@ * Tests for {@link StarTreeMapper}. */ public class StarTreeMapperTests extends MapperTestCase { + FeatureFlags.TestUtils.FlagLock ffLock = null; @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + ffLock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + ffLock.close(); } @Override diff --git a/server/src/test/java/org/opensearch/index/store/StoreTests.java b/server/src/test/java/org/opensearch/index/store/StoreTests.java index 542f95a4894e2..22f7f1fd31997 100644 --- a/server/src/test/java/org/opensearch/index/store/StoreTests.java +++ b/server/src/test/java/org/opensearch/index/store/StoreTests.java @@ -72,7 +72,6 @@ import org.opensearch.common.lucene.LuceneTests; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.io.stream.InputStreamStreamInput; import org.opensearch.core.common.io.stream.OutputStreamStreamOutput; @@ -90,7 +89,6 @@ import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.indices.store.TransportNodesListShardStoreMetadataHelper.StoreFilesMetadata; import org.opensearch.test.DummyShardLock; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; import org.opensearch.test.OpenSearchTestCase; import org.hamcrest.Matchers; @@ -113,6 +111,7 @@ import java.util.concurrent.atomic.AtomicInteger; import static java.util.Collections.unmodifiableMap; +import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.index.seqno.SequenceNumbers.LOCAL_CHECKPOINT_KEY; import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; import static org.opensearch.test.VersionUtils.randomVersion; @@ -1282,6 +1281,7 @@ public void testSegmentReplicationDiff() { assertTrue(diff.identical.isEmpty()); } + @LockFeatureFlag(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY) @SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag") public void testReadSegmentsFromOldIndices() throws Exception { int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major; @@ -1291,7 +1291,6 @@ public void testReadSegmentsFromOldIndices() throws Exception { Store store = null; try { - FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( "index", Settings.builder() diff --git a/server/src/test/java/org/opensearch/node/NodeTests.java b/server/src/test/java/org/opensearch/node/NodeTests.java index 2f769dbd51b0a..999586f4f8639 100644 --- a/server/src/test/java/org/opensearch/node/NodeTests.java +++ b/server/src/test/java/org/opensearch/node/NodeTests.java @@ -42,7 +42,6 @@ import org.opensearch.common.network.NetworkModule; import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsException; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.breaker.CircuitBreaker; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; import org.opensearch.core.common.transport.BoundTransportAddress; @@ -69,7 +68,6 @@ import org.opensearch.telemetry.TelemetrySettings; import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.tracing.Tracer; -import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.InternalTestCluster; import org.opensearch.test.MockHttpTransport; import org.opensearch.test.NodeRoles; @@ -94,6 +92,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; +import static org.opensearch.common.util.FeatureFlags.TELEMETRY; import static org.opensearch.test.NodeRoles.addRoles; import static org.opensearch.test.NodeRoles.dataNode; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @@ -436,11 +435,11 @@ public void testTelemetryAwarePlugins() throws IOException { } } + @LockFeatureFlag(TELEMETRY) public void testTelemetryPluginShouldNOTImplementTelemetryAwarePlugin() throws IOException { Settings.Builder settings = baseSettings(); List> plugins = basePlugins(); plugins.add(MockTelemetryPlugin.class); - FeatureFlagSetter.set(FeatureFlags.TELEMETRY); settings.put(TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), true); assertThrows(IllegalStateException.class, () -> new MockNode(settings.build(), plugins)); } diff --git a/server/src/test/java/org/opensearch/search/SearchModuleTests.java b/server/src/test/java/org/opensearch/search/SearchModuleTests.java index d78393e917b2f..658e7bd2297c4 100644 --- a/server/src/test/java/org/opensearch/search/SearchModuleTests.java +++ b/server/src/test/java/org/opensearch/search/SearchModuleTests.java @@ -34,7 +34,6 @@ import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.util.CharsRefBuilder; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; @@ -438,7 +437,6 @@ public void testConcurrentQueryPhaseSearcher() { QueryPhase queryPhase = searchModule.getQueryPhase(); assertTrue(queryPhase.getQueryPhaseSearcher() instanceof QueryPhaseSearcherWrapper); assertTrue(queryPhase.getQueryPhaseSearcher().aggregationProcessor(searchContext) instanceof ConcurrentAggregationProcessor); - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); } public void testPluginQueryPhaseSearcher() { @@ -454,7 +452,6 @@ public Optional getQueryPhaseSearcher() { TestSearchContext searchContext = new TestSearchContext(null); assertEquals(queryPhaseSearcher, queryPhase.getQueryPhaseSearcher()); assertTrue(queryPhaseSearcher.aggregationProcessor(searchContext) instanceof DefaultAggregationProcessor); - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); } public void testMultiplePluginRegisterQueryPhaseSearcher() { diff --git a/server/src/test/java/org/opensearch/search/SearchServiceStarTreeTests.java b/server/src/test/java/org/opensearch/search/SearchServiceStarTreeTests.java index 95c877bfce0a8..f5faef3d13b73 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceStarTreeTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceStarTreeTests.java @@ -15,7 +15,6 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.Rounding; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.Strings; import org.opensearch.index.IndexService; import org.opensearch.index.codec.composite.CompositeIndexFieldInfo; @@ -68,6 +67,7 @@ import java.util.List; import java.util.Set; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.search.aggregations.AggregationBuilders.dateHistogram; import static org.opensearch.search.aggregations.AggregationBuilders.max; import static org.opensearch.search.aggregations.AggregationBuilders.medianAbsoluteDeviation; @@ -91,8 +91,8 @@ public class SearchServiceStarTreeTests extends OpenSearchSingleNodeTestCase { /** * Test query parsing for non-nested metric aggregations, with/without numeric term query */ + @LockFeatureFlag(STAR_TREE_INDEX) public void testQueryParsingForMetricAggregations() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); setStarTreeIndexSetting("true"); Settings settings = Settings.builder() @@ -242,8 +242,8 @@ public void testQueryParsingForMetricAggregations() throws IOException { /** * Test query parsing for date histogram aggregations, with/without numeric term query */ + @LockFeatureFlag(STAR_TREE_INDEX) public void testQueryParsingForDateHistogramAggregations() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); setStarTreeIndexSetting("true"); Settings settings = Settings.builder() @@ -491,8 +491,8 @@ public void testCacheCreationInStarTreeQueryContext() throws IOException { /** * Test query parsing for date histogram aggregations on star-tree index when @timestamp field does not exist */ + @LockFeatureFlag(STAR_TREE_INDEX) public void testInvalidQueryParsingForDateHistogramAggregations() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); setStarTreeIndexSetting("true"); Settings settings = Settings.builder() @@ -545,8 +545,8 @@ public void testInvalidQueryParsingForDateHistogramAggregations() throws IOExcep /** * Test query parsing for bucket aggregations, with/without numeric term query */ + @LockFeatureFlag(STAR_TREE_INDEX) public void testQueryParsingForBucketAggregations() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); setStarTreeIndexSetting("true"); Settings settings = Settings.builder() diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java index a374e2f5653b9..81733fd723a1c 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java @@ -28,7 +28,6 @@ import org.apache.lucene.tests.index.RandomIndexWriter; import org.opensearch.common.Rounding; import org.opensearch.common.lucene.Lucene; -import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.codec.composite.CompositeIndexFieldInfo; @@ -59,6 +58,7 @@ import java.util.List; import java.util.Random; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.index.codec.composite912.datacube.startree.AbstractStarTreeDVFormatTests.topMapping; import static org.opensearch.search.aggregations.AggregationBuilders.avg; import static org.opensearch.search.aggregations.AggregationBuilders.count; @@ -69,6 +69,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class DateHistogramAggregatorTests extends DateHistogramAggregatorTestCase { + private static FeatureFlags.TestUtils.FlagLock fflock = null; private static final String TIMESTAMP_FIELD = "@timestamp"; private static final MappedFieldType TIMESTAMP_FIELD_TYPE = new DateFieldMapper.DateFieldType(TIMESTAMP_FIELD); @@ -80,12 +81,12 @@ public class DateHistogramAggregatorTests extends DateHistogramAggregatorTestCas @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec() { diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java index 2ca9f6b592a0d..ff86272621add 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java @@ -31,7 +31,6 @@ import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.NumericUtils; import org.opensearch.common.lucene.Lucene; -import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.codec.composite.CompositeIndexFieldInfo; import org.opensearch.index.codec.composite.CompositeIndexReader; @@ -60,6 +59,7 @@ import java.util.List; import java.util.Random; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.search.aggregations.AggregationBuilders.avg; import static org.opensearch.search.aggregations.AggregationBuilders.count; import static org.opensearch.search.aggregations.AggregationBuilders.max; @@ -69,6 +69,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class KeywordTermsAggregatorTests extends AggregatorTestCase { + private static FeatureFlags.TestUtils.FlagLock fflock = null; final static String STATUS = "status"; final static String SIZE = "size"; final static String CLIENTIP = "clientip"; @@ -81,12 +82,12 @@ public class KeywordTermsAggregatorTests extends AggregatorTestCase { @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec() { diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java index 6e10562c3a846..fc16060330f6a 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java @@ -91,6 +91,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.search.aggregations.AggregationBuilders.avg; import static org.opensearch.search.aggregations.AggregationBuilders.count; import static org.opensearch.search.aggregations.AggregationBuilders.max; @@ -101,19 +102,19 @@ import static org.mockito.Mockito.when; public class MetricAggregatorTests extends AggregatorTestCase { - + private static FeatureFlags.TestUtils.FlagLock fflock = null; private static final String FIELD_NAME = "field"; private static final NumberFieldMapper.NumberType DEFAULT_FIELD_TYPE = NumberFieldMapper.NumberType.LONG; private static final MappedFieldType DEFAULT_MAPPED_FIELD = new NumberFieldMapper.NumberFieldType(FIELD_NAME, DEFAULT_FIELD_TYPE); @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec( diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java index d3cb2d17e7c16..b28a54aef9a7b 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java @@ -27,7 +27,6 @@ import org.apache.lucene.tests.index.RandomIndexWriter; import org.apache.lucene.util.NumericUtils; import org.opensearch.common.lucene.Lucene; -import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.codec.composite.CompositeIndexFieldInfo; @@ -54,6 +53,7 @@ import java.util.List; import java.util.Random; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.index.codec.composite912.datacube.startree.AbstractStarTreeDVFormatTests.topMapping; import static org.opensearch.search.aggregations.AggregationBuilders.avg; import static org.opensearch.search.aggregations.AggregationBuilders.count; @@ -64,6 +64,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class NumericTermsAggregatorTests extends AggregatorTestCase { + private static FeatureFlags.TestUtils.FlagLock fflock = null; final static String STATUS = "status"; final static String SIZE = "size"; private static final MappedFieldType STATUS_FIELD_TYPE = new NumberFieldMapper.NumberFieldType( @@ -74,12 +75,12 @@ public class NumericTermsAggregatorTests extends AggregatorTestCase { @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec() { diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java index 7282b0fafb8aa..67bac90c52ea5 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java @@ -24,7 +24,6 @@ import org.apache.lucene.tests.index.RandomIndexWriter; import org.apache.lucene.util.FixedBitSet; import org.opensearch.common.lucene.Lucene; -import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.codec.composite.CompositeIndexFieldInfo; @@ -57,9 +56,11 @@ import org.mockito.Mockito; +import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX; import static org.opensearch.index.codec.composite912.datacube.startree.AbstractStarTreeDVFormatTests.topMapping; public class StarTreeFilterTests extends AggregatorTestCase { + private static FeatureFlags.TestUtils.FlagLock fflock = null; private static final String FIELD_NAME = "field"; private static final String SNDV = "sndv"; @@ -78,12 +79,12 @@ public class StarTreeFilterTests extends AggregatorTestCase { @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec(int maxLeafDoc, boolean skipStarNodeCreationForSDVDimension) { From 888290779bb0a8042df16f549376d12e8614b089 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 20 Mar 2025 11:00:35 -0700 Subject: [PATCH 06/10] Add changelog entry Signed-off-by: Finn Carroll --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbd8d2c427b29..144da201ae26c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Fixed - Fix bytes parameter on `_cat/recovery` ([#17598](https://github.com/opensearch-project/OpenSearch/pull/17598)) +- Fix slow performance of FeatureFlag checks ([#17611](https://github.com/opensearch-project/OpenSearch/pull/17611)) ### Security From dc5db80f591885cfd1da3cec705601f2d6119f9d Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 18 Mar 2025 14:52:29 -0700 Subject: [PATCH 07/10] Fix lingering test failures - Add missing LockFeatureFlag annotations. - Cannot use annotation in tests which expect exception thrown. - SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY has no setting? Adding. - Flight server tests need flag enabled on setup. Signed-off-by: Finn Carroll --- .../bootstrap/FlightClientManagerTests.java | 5 +++- .../opensearch/common/util/FeatureFlags.java | 9 ++++++ .../MetadataIndexTemplateServiceTests.java | 4 +++ .../routing/OperationRoutingTests.java | 4 +-- .../index/mapper/ObjectMapperTests.java | 28 ++++++++++--------- .../opensearch/test/OpenSearchTestCase.java | 6 +--- 6 files changed, 35 insertions(+), 21 deletions(-) diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java index f8b04197805d1..d2a4458104682 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java @@ -24,6 +24,7 @@ import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.transport.BoundTransportAddress; import org.opensearch.core.common.transport.TransportAddress; @@ -63,6 +64,7 @@ @SuppressWarnings("unchecked") public class FlightClientManagerTests extends OpenSearchTestCase { + private static FeatureFlags.TestUtils.FlagLock ffLock = null; private static BufferAllocator allocator; private static EventLoopGroup elg; @@ -77,13 +79,13 @@ public class FlightClientManagerTests extends OpenSearchTestCase { @BeforeClass public static void setupClass() throws Exception { + ffLock = new FeatureFlags.TestUtils.FlagLock(ARROW_STREAMS); ServerConfig.init(Settings.EMPTY); allocator = new RootAllocator(); elg = ServerConfig.createELG("test-grpc-worker-elg", NettyRuntime.availableProcessors() * 2); executorService = ServerConfig.createELG("test-grpc-worker", NettyRuntime.availableProcessors() * 2); } - @LockFeatureFlag(ARROW_STREAMS) @Override public void setUp() throws Exception { super.setUp(); @@ -175,6 +177,7 @@ private DiscoveryNode createNode(String nodeId, String host, int port) throws Ex @AfterClass public static void tearClass() { allocator.close(); + ffLock.close(); } public void testGetFlightClientForExistingNode() { diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index df78487f5c459..d0a8466bd0f0f 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -42,6 +42,11 @@ public class FeatureFlags { */ public static final String SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY = FEATURE_FLAG_PREFIX + "searchable_snapshot.extended_compatibility.enabled"; + public static final Setting SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_SETTING = Setting.boolSetting( + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, + false, + Property.NodeScope + ); /** * Gates the functionality of extensions. @@ -162,6 +167,10 @@ static class FeatureFlagsImpl { put(READER_WRITER_SPLIT_EXPERIMENTAL_SETTING, READER_WRITER_SPLIT_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)); put(TERM_VERSION_PRECOMMIT_ENABLE_SETTING, TERM_VERSION_PRECOMMIT_ENABLE_SETTING.getDefault(Settings.EMPTY)); put(ARROW_STREAMS_SETTING, ARROW_STREAMS_SETTING.getDefault(Settings.EMPTY)); + put( + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_SETTING, + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_SETTING.getDefault(Settings.EMPTY) + ); } }; diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index 37ad8bdcaea67..a91e724ec279d 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -972,10 +972,12 @@ public void onFailure(Exception e) { ); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testPutGlobalV2TemplateWhichProvidesContextWithSpecificVersion() throws Exception { verifyTemplateCreationUsingContext("1"); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testPutGlobalV2TemplateWhichProvidesContextWithLatestVersion() throws Exception { verifyTemplateCreationUsingContext("_latest"); } @@ -1014,6 +1016,7 @@ public void testModifySystemTemplateViaUnknownSource() throws Exception { ); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testResolveSettingsWithContextVersion() throws Exception { ClusterService clusterService = node().injector().getInstance(ClusterService.class); final String indexTemplateName = verifyTemplateCreationUsingContext("1"); @@ -1022,6 +1025,7 @@ public void testResolveSettingsWithContextVersion() throws Exception { assertThat(settings.get("index.codec"), equalTo(CodecService.BEST_COMPRESSION_CODEC)); } + @LockFeatureFlag(APPLICATION_BASED_CONFIGURATION_TEMPLATES) public void testResolveSettingsWithContextLatest() throws Exception { ClusterService clusterService = node().injector().getInstance(ClusterService.class); final String indexTemplateName = verifyTemplateCreationUsingContext(Context.LATEST_VERSION); diff --git a/server/src/test/java/org/opensearch/cluster/routing/OperationRoutingTests.java b/server/src/test/java/org/opensearch/cluster/routing/OperationRoutingTests.java index 8cfdcce45c523..e06f0408e6fc9 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/OperationRoutingTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/OperationRoutingTests.java @@ -72,6 +72,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; +import static org.opensearch.common.util.FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; @@ -1058,9 +1059,9 @@ public void testSearchableSnapshotPrimaryDefault() throws Exception { } } + @LockFeatureFlag(WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG) @SuppressForbidden(reason = "feature flag overrides") public void testPartialIndexPrimaryDefault() throws Exception { - System.setProperty(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG, "true"); final int numIndices = 1; final int numShards = 2; final int numReplicas = 2; @@ -1116,7 +1117,6 @@ public void testPartialIndexPrimaryDefault() throws Exception { } finally { IOUtils.close(clusterService); terminate(threadPool); - System.setProperty(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG, "false"); } } diff --git a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java index b5f3e8fb8331b..8b2eb67481abf 100644 --- a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java @@ -35,6 +35,7 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.compress.CompressedXContent; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.unit.ByteSizeUnit; @@ -498,7 +499,6 @@ public void testDerivedFields() throws Exception { assertEquals("date", mapper.typeName()); } - @LockFeatureFlag(STAR_TREE_INDEX) public void testCompositeFields() throws Exception { String mapping = XContentFactory.jsonBuilder() .startObject() @@ -556,18 +556,20 @@ public void testCompositeFields() throws Exception { ex.getMessage() ); - DocumentMapper documentMapper = createIndex("test", settings).mapperService() - .documentMapperParser() - .parse("tweet", new CompressedXContent(mapping)); - - Mapper mapper = documentMapper.root().getMapper("startree"); - assertTrue(mapper instanceof StarTreeMapper); - StarTreeMapper starTreeMapper = (StarTreeMapper) mapper; - assertEquals("star_tree", starTreeMapper.fieldType().typeName()); - // Check that field in properties was parsed correctly as well - mapper = documentMapper.root().getMapper("@timestamp"); - assertNotNull(mapper); - assertEquals("date", mapper.typeName()); + FeatureFlags.TestUtils.with(STAR_TREE_INDEX, () -> { + DocumentMapper documentMapper = createIndex("test", settings).mapperService() + .documentMapperParser() + .parse("tweet", new CompressedXContent(mapping)); + + Mapper mapper = documentMapper.root().getMapper("startree"); + assertTrue(mapper instanceof StarTreeMapper); + StarTreeMapper starTreeMapper = (StarTreeMapper) mapper; + assertEquals("star_tree", starTreeMapper.fieldType().typeName()); + // Check that field in properties was parsed correctly as well + mapper = documentMapper.root().getMapper("@timestamp"); + assertNotNull(mapper); + assertEquals("date", mapper.typeName()); + }); } public void testNestedIsParent() throws Exception { diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java index b06947e70ba86..31e282891514b 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java @@ -247,11 +247,7 @@ public abstract class OpenSearchTestCase extends LuceneTestCase { * Enables and locks a feature flag for the duration of the test case. * This annotation guarantees the feature flag will be enabled for the duration of * the test case. Feature flag is returned to previous value on test exit. - * Usage: - * @LockFeatureFlag("example.featureflag.setting.key.enabled") - * public void testFeatureFlagExample() { - * // Test flag dependant feature - * } + * Usage: LockFeatureFlag("example.featureflag.setting.key.enabled") */ @Retention(RetentionPolicy.RUNTIME) @Target({ ElementType.METHOD }) From c210e90d788a5ba8d4bfe7647d3c34b8cce60346 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Sun, 23 Mar 2025 15:42:05 -0700 Subject: [PATCH 08/10] Remove concurrency primitives JUnit does not run tests in parallel on the same JVM so these are not necessary. Additionally rename to `FlagWriteLock` for clarity. Signed-off-by: Finn Carroll --- .../bootstrap/FlightClientManagerTests.java | 4 +- .../flight/bootstrap/FlightServiceTests.java | 4 +- .../opensearch/common/util/FeatureFlags.java | 112 +++++++++--------- .../common/util/FeatureFlagTests.java | 45 +------ .../extensions/ExtensionsManagerTests.java | 4 +- .../AbstractStarTreeDVFormatTests.java | 4 +- .../index/mapper/StarTreeMapperTests.java | 4 +- .../DateHistogramAggregatorTests.java | 4 +- .../startree/KeywordTermsAggregatorTests.java | 4 +- .../startree/MetricAggregatorTests.java | 4 +- .../startree/NumericTermsAggregatorTests.java | 4 +- .../startree/StarTreeFilterTests.java | 4 +- .../opensearch/test/OpenSearchTestCase.java | 10 +- 13 files changed, 82 insertions(+), 125 deletions(-) diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java index d2a4458104682..ce2f0df7f5f55 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightClientManagerTests.java @@ -64,7 +64,7 @@ @SuppressWarnings("unchecked") public class FlightClientManagerTests extends OpenSearchTestCase { - private static FeatureFlags.TestUtils.FlagLock ffLock = null; + private static FeatureFlags.TestUtils.FlagWriteLock ffLock = null; private static BufferAllocator allocator; private static EventLoopGroup elg; @@ -79,7 +79,7 @@ public class FlightClientManagerTests extends OpenSearchTestCase { @BeforeClass public static void setupClass() throws Exception { - ffLock = new FeatureFlags.TestUtils.FlagLock(ARROW_STREAMS); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(ARROW_STREAMS); ServerConfig.init(Settings.EMPTY); allocator = new RootAllocator(); elg = ServerConfig.createELG("test-grpc-worker-elg", NettyRuntime.availableProcessors() * 2); diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java index e85d5a3893e45..e7bf402167321 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java @@ -36,7 +36,7 @@ import static org.mockito.Mockito.when; public class FlightServiceTests extends OpenSearchTestCase { - FeatureFlags.TestUtils.FlagLock ffLock = null; + FeatureFlags.TestUtils.FlagWriteLock ffLock = null; private Settings settings; private ClusterService clusterService; @@ -48,7 +48,7 @@ public class FlightServiceTests extends OpenSearchTestCase { @Override public void setUp() throws Exception { super.setUp(); - ffLock = new FeatureFlags.TestUtils.FlagLock(ARROW_STREAMS); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(ARROW_STREAMS); int availablePort = getBasePort(9500) + port.addAndGet(1); settings = Settings.EMPTY; localNode = createNode(availablePort); diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index d0a8466bd0f0f..39010f46f9c01 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -12,14 +12,14 @@ import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; -import java.util.Map; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.ReentrantLock; /** * Feature flags are used to gate the visibility/availability of incomplete features. For more information, see * https://featureflags.io/feature-flag-introduction/ - * Due to their specific use case, feature flag settings have several additional properties enforced by convention and code: + * Due to their use case, feature flag settings have several additional properties enforced by convention and code: * - Feature flags are boolean settings. * - Feature flags are static settings. * - Feature flags are globally available. @@ -202,11 +202,11 @@ void initializeFeatureFlags(Settings openSearchSettings) { /** * Set all feature flags according to setting defaults. * Overwrites existing entries in feature flags map. - * Skips flags which are locked according to TestUtils.FlagLock. + * Skips flags which are write locked according to TestUtils.FlagLock. */ private void initFromDefaults() { for (Setting ff : featureFlags.keySet()) { - if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; + if (TestUtils.FlagWriteLock.isLocked(ff.getKey())) continue; featureFlags.put(ff, ff.getDefault(Settings.EMPTY)); } } @@ -215,11 +215,11 @@ private void initFromDefaults() { * Update feature flags according to JVM system properties. * Feature flags are true if system property is set as "true" (case-insensitive). Else feature set to false. * Overwrites existing value if system property exists. - * Skips flags which are locked according to TestUtils.FlagLock. + * Skips flags which are write locked according to TestUtils.FlagLock. */ private void initFromSysProperties() { for (Setting ff : featureFlags.keySet()) { - if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; + if (TestUtils.FlagWriteLock.isLocked(ff.getKey())) continue; String prop = System.getProperty(ff.getKey()); if (prop != null) { featureFlags.put(ff, Boolean.valueOf(prop)); @@ -228,14 +228,18 @@ private void initFromSysProperties() { } /** - * @param featureFlagName feature flag setting key - * @return true if feature enabled - else false + * Update feature flags in ALL_FEATURE_FLAG_SETTINGS according to provided settings. + * Overwrites existing entries in feature flags map. + * Skips flags which are write locked according to TestUtils.FlagLock. + * @param settings settings to update feature flags from */ - boolean isEnabled(String featureFlagName) { + private void initFromSettings(Settings settings) { for (Setting ff : featureFlags.keySet()) { - if (ff.getKey().equals(featureFlagName)) return featureFlags.get(ff); + if (settings.hasValue(ff.getKey())) { + if (TestUtils.FlagWriteLock.isLocked(ff.getKey())) continue; + featureFlags.put(ff, settings.getAsBoolean(ff.getKey(), ff.getDefault(settings))); + } } - return false; } /** @@ -248,27 +252,23 @@ boolean isEnabled(Setting ff) { } /** - * @param featureFlagName feature flag key to set - * @param value value for flag + * @param featureFlagName feature flag setting key + * @return true if feature enabled - else false */ - void set(String featureFlagName, Boolean value) { + boolean isEnabled(String featureFlagName) { for (Setting ff : featureFlags.keySet()) { - if (ff.getKey().equals(featureFlagName)) featureFlags.put(ff, value); + if (ff.getKey().equals(featureFlagName)) return featureFlags.get(ff); } + return false; } /** - * Update feature flags in ALL_FEATURE_FLAG_SETTINGS according to provided settings. - * Overwrites existing entries in feature flags map. - * Skips flags which are locked according to TestUtils.FlagLock. - * @param settings settings to update feature flags from + * @param featureFlagName feature flag to set + * @param value value for flag */ - private void initFromSettings(Settings settings) { + void set(String featureFlagName, Boolean value) { for (Setting ff : featureFlags.keySet()) { - if (settings.hasValue(ff.getKey())) { - if (TestUtils.FlagLock.isLocked(ff.getKey())) continue; - featureFlags.put(ff, settings.getAsBoolean(ff.getKey(), ff.getDefault(settings))); - } + if (ff.getKey().equals(featureFlagName)) featureFlags.put(ff, value); } } } @@ -291,49 +291,51 @@ public static boolean isEnabled(Setting featureFlag) { } /** - * Provides feature flag write access and synchronization for test use cases. + * Provides feature flag write access for test use cases. * To enable a feature flag for a single test case see @LockFeatureFlag annotation. * For more fine grain control us TestUtils.with() or explicitly construct a new FlagLock(). + * Note: JUnit will not run test cases concurrently within a suite by default. + * Similarly test suites are forked and run in a separate JVM environment. + * As such these utility methods do not provide any thread safety. */ public static class TestUtils { /** - * Maintains an internal map of re-entrant locks corresponding to feature flags. - * Constructing a new FlagLock sets and locks the value of that feature flag. - * On unlock or close the flag is unlocked and reset to its previous value. + * AutoCloseable helper which sets a feature flag and makes it immutable for the lifetime of the lock. + * Throws an exception if two locks exist for the same flag as we should never reach this state. + * Initializing two write locks for the same flag throws a RuntimeException. */ - public static class FlagLock implements AutoCloseable { - private static final Map flagLocks = new ConcurrentHashMap<>(); - private final String flagKey; + public static class FlagWriteLock implements AutoCloseable { + private static final Set writeLocks = new HashSet<>(); + private final String flag; private final Boolean prev; - public static boolean isLocked(String flagKey) { - if (!flagLocks.containsKey(flagKey)) return false; - return flagLocks.get(flagKey).isLocked(); + public static boolean isLocked(String flag) { + return writeLocks.contains(flag); } - public FlagLock(String flagKey) { - this(flagKey, true); + public FlagWriteLock(String flag) { + this(flag, true); } - public FlagLock(String flagKey, Boolean value) { - this.flagKey = flagKey; - this.prev = featureFlagsImpl.isEnabled(flagKey); - if (flagLocks.containsKey(flagKey) == false) { - flagLocks.put(flagKey, new ReentrantLock()); + public FlagWriteLock(String flag, Boolean value) { + if (writeLocks.contains(flag)) { + throw new RuntimeException("Cannot initialize second write lock for feature flag: " + flag); } - flagLocks.get(flagKey).lock(); - featureFlagsImpl.set(flagKey, value); + this.flag = flag; + this.prev = featureFlagsImpl.isEnabled(flag); + writeLocks.add(flag); + featureFlagsImpl.set(flag, value); } public void unlock() { - featureFlagsImpl.set(flagKey, prev); - flagLocks.get(flagKey).unlock(); + featureFlagsImpl.set(flag, prev); + writeLocks.remove(flag); } @Override public void close() { - featureFlagsImpl.set(flagKey, prev); - flagLocks.get(flagKey).unlock(); + featureFlagsImpl.set(flag, prev); + writeLocks.remove(flag); } } @@ -346,19 +348,19 @@ public interface ThrowingRunnable { } /** - * Helper to synchronize a runnable test action on a feature flag to avoid race conditions when tests are run in parallel. - * Sets the feature flag back to its previous value after executing the test action. - * @param key feature flag setting key. + * Executes runnable test action with the provided feature flag enabled. + * Returns feature flag to previous value. + * @param flag feature flag setting. * @param action critical section to run while feature flag is set. */ - public static void with(String key, ThrowingRunnable action) throws Exception { - try (FlagLock ignored = new FlagLock(key)) { + public static void with(String flag, ThrowingRunnable action) throws Exception { + try (FlagWriteLock ignored = new FlagWriteLock(flag)) { action.run(); } } - public static void with(String key, Boolean value, ThrowingRunnable action) throws Exception { - try (FlagLock ignored = new FlagLock(key, value)) { + public static void with(String flag, Boolean value, ThrowingRunnable action) throws Exception { + try (FlagWriteLock ignored = new FlagWriteLock(flag, value)) { action.run(); } } diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index eae3c710d0efa..f3751e98f5b60 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -12,7 +12,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; -import static java.lang.Thread.sleep; import static org.opensearch.common.util.FeatureFlags.FEATURE_FLAG_PREFIX; public class FeatureFlagTests extends OpenSearchTestCase { @@ -105,7 +104,7 @@ public void testFeatureDoesNotExist() { */ public void testLockFeatureFlagWithFlagLock() { - try (FeatureFlags.TestUtils.FlagLock ignore = new FeatureFlags.TestUtils.FlagLock(TEST_FLAG)) { + try (FeatureFlags.TestUtils.FlagWriteLock ignore = new FeatureFlags.TestUtils.FlagWriteLock(TEST_FLAG)) { assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked @@ -126,46 +125,4 @@ public void testLockFeatureFlagAnnotation() { FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked } - - // Attempt to set TEST_FLAG false every 2ms for a maximum of 2seconds. - private Thread attackTestFlag() { - Thread attackerThread = new Thread(() -> { - long endTime = System.currentTimeMillis() + 2000; - while (System.currentTimeMillis() < endTime) { - try { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); - sleep(2); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - break; - } - } - }); - - attackerThread.start(); - return attackerThread; - } - - public void testAttackFlagLock() { - Thread attacker = attackTestFlag(); - try (FeatureFlags.TestUtils.FlagLock ignore = new FeatureFlags.TestUtils.FlagLock(TEST_FLAG)) { - for (int i = 0; i < 100; i++) { - assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); - sleep(2); - } - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - attacker.interrupt(); - } - - @LockFeatureFlag(TEST_FLAG) - public void testAttackAnnotationFlagLock() throws InterruptedException { - Thread attacker = attackTestFlag(); - for (int i = 0; i < 100; i++) { - assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); - sleep(2); - } - attacker.interrupt(); - } } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 7d2625d218d9e..df03120d06fb0 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -86,7 +86,7 @@ import static org.mockito.Mockito.when; public class ExtensionsManagerTests extends OpenSearchTestCase { - private static FeatureFlags.TestUtils.FlagLock ffLock = null; + private static FeatureFlags.TestUtils.FlagWriteLock ffLock = null; private TransportService transportService; private ActionModule actionModule; private DynamicActionRegistry dynamicActionRegistry; @@ -109,7 +109,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { @Before public void setup() throws Exception { - ffLock = new FeatureFlags.TestUtils.FlagLock(EXTENSIONS); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(EXTENSIONS); Settings settings = Settings.builder().put("cluster.name", "test").build(); transport = new MockNioTransport( settings, diff --git a/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java b/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java index f06f747a72880..dd754845d1f58 100644 --- a/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java +++ b/server/src/test/java/org/opensearch/index/codec/composite912/datacube/startree/AbstractStarTreeDVFormatTests.java @@ -51,7 +51,7 @@ */ @LuceneTestCase.SuppressSysoutChecks(bugUrl = "we log a lot on purpose") public abstract class AbstractStarTreeDVFormatTests extends BaseDocValuesFormatTestCase { - private static FeatureFlags.TestUtils.FlagLock ffLock = null; + private static FeatureFlags.TestUtils.FlagWriteLock ffLock = null; MapperService mapperService = null; StarTreeFieldConfiguration.StarTreeBuildMode buildMode; @@ -69,7 +69,7 @@ public static Collection parameters() { @BeforeClass public static void createMapper() { - ffLock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @AfterClass diff --git a/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java index 140b2e7694546..b355aaea24121 100644 --- a/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java @@ -54,11 +54,11 @@ * Tests for {@link StarTreeMapper}. */ public class StarTreeMapperTests extends MapperTestCase { - FeatureFlags.TestUtils.FlagLock ffLock = null; + FeatureFlags.TestUtils.FlagWriteLock ffLock = null; @Before public void setup() { - ffLock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java index 81733fd723a1c..0a6a775ac57b5 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DateHistogramAggregatorTests.java @@ -69,7 +69,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class DateHistogramAggregatorTests extends DateHistogramAggregatorTestCase { - private static FeatureFlags.TestUtils.FlagLock fflock = null; + private static FeatureFlags.TestUtils.FlagWriteLock fflock = null; private static final String TIMESTAMP_FIELD = "@timestamp"; private static final MappedFieldType TIMESTAMP_FIELD_TYPE = new DateFieldMapper.DateFieldType(TIMESTAMP_FIELD); @@ -81,7 +81,7 @@ public class DateHistogramAggregatorTests extends DateHistogramAggregatorTestCas @Before public void setup() { - fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java index ff86272621add..0cfaa52103539 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/KeywordTermsAggregatorTests.java @@ -69,7 +69,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class KeywordTermsAggregatorTests extends AggregatorTestCase { - private static FeatureFlags.TestUtils.FlagLock fflock = null; + private static FeatureFlags.TestUtils.FlagWriteLock fflock = null; final static String STATUS = "status"; final static String SIZE = "size"; final static String CLIENTIP = "clientip"; @@ -82,7 +82,7 @@ public class KeywordTermsAggregatorTests extends AggregatorTestCase { @Before public void setup() { - fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java index fc16060330f6a..0f0db9907d381 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java @@ -102,14 +102,14 @@ import static org.mockito.Mockito.when; public class MetricAggregatorTests extends AggregatorTestCase { - private static FeatureFlags.TestUtils.FlagLock fflock = null; + private static FeatureFlags.TestUtils.FlagWriteLock fflock = null; private static final String FIELD_NAME = "field"; private static final NumberFieldMapper.NumberType DEFAULT_FIELD_TYPE = NumberFieldMapper.NumberType.LONG; private static final MappedFieldType DEFAULT_MAPPED_FIELD = new NumberFieldMapper.NumberFieldType(FIELD_NAME, DEFAULT_FIELD_TYPE); @Before public void setup() { - fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java index b28a54aef9a7b..7cf83a73de052 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/NumericTermsAggregatorTests.java @@ -64,7 +64,7 @@ import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS; public class NumericTermsAggregatorTests extends AggregatorTestCase { - private static FeatureFlags.TestUtils.FlagLock fflock = null; + private static FeatureFlags.TestUtils.FlagWriteLock fflock = null; final static String STATUS = "status"; final static String SIZE = "size"; private static final MappedFieldType STATUS_FIELD_TYPE = new NumberFieldMapper.NumberFieldType( @@ -75,7 +75,7 @@ public class NumericTermsAggregatorTests extends AggregatorTestCase { @Before public void setup() { - fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java index 67bac90c52ea5..cd2943f23be7a 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java @@ -60,7 +60,7 @@ import static org.opensearch.index.codec.composite912.datacube.startree.AbstractStarTreeDVFormatTests.topMapping; public class StarTreeFilterTests extends AggregatorTestCase { - private static FeatureFlags.TestUtils.FlagLock fflock = null; + private static FeatureFlags.TestUtils.FlagWriteLock fflock = null; private static final String FIELD_NAME = "field"; private static final String SNDV = "sndv"; @@ -79,7 +79,7 @@ public class StarTreeFilterTests extends AggregatorTestCase { @Before public void setup() { - fflock = new FeatureFlags.TestUtils.FlagLock(STAR_TREE_INDEX); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(STAR_TREE_INDEX); } @After diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java index 31e282891514b..2525aef903298 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java @@ -244,9 +244,8 @@ public abstract class OpenSearchTestCase extends LuceneTestCase { /** * Define LockFeatureFlag annotation for unit tests. - * Enables and locks a feature flag for the duration of the test case. - * This annotation guarantees the feature flag will be enabled for the duration of - * the test case. Feature flag is returned to previous value on test exit. + * Enables and make a flag immutable for the duration of the test case. + * Flag returned to previous value on test exit. * Usage: LockFeatureFlag("example.featureflag.setting.key.enabled") */ @Retention(RetentionPolicy.RUNTIME) @@ -257,8 +256,7 @@ public abstract class OpenSearchTestCase extends LuceneTestCase { public static class AnnotatedFeatureFlagRule implements TestRule { /** - * If test is annotated with @LockFeatureFlag wrap the base Statement with the appropriate - * FeatureFlags.TestUtils.FlagLock to ensure test is synchronized on that flag. + * Wrap base test case with an * @param base test case to execute. * @param description annotated test description. */ @@ -272,7 +270,7 @@ public Statement apply(Statement base, Description description) { return new Statement() { @Override public void evaluate() throws Throwable { - try (FeatureFlags.TestUtils.FlagLock ignored = new FeatureFlags.TestUtils.FlagLock(flagKey)) { + try (FeatureFlags.TestUtils.FlagWriteLock ignored = new FeatureFlags.TestUtils.FlagWriteLock(flagKey)) { base.evaluate(); } } From 9a35bf11c780ed84644edf4c5509cfa0e55eb3d7 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Mon, 24 Mar 2025 09:14:17 -0700 Subject: [PATCH 09/10] Fix flight service IT. Address ff contant nit. Signed-off-by: Finn Carroll --- .../opensearch/arrow/flight/bootstrap/FlightServiceTests.java | 1 + .../main/java/org/opensearch/common/util/FeatureFlags.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java index e7bf402167321..d8f5d5ba6b45b 100644 --- a/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java +++ b/plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/FlightServiceTests.java @@ -148,6 +148,7 @@ public void testLifecycleStateTransitions() throws Exception { @Override public void tearDown() throws Exception { super.tearDown(); + ffLock.close(); } private DiscoveryNode createNode(int port) throws Exception { diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 39010f46f9c01..a17d5407dd3b8 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -28,8 +28,8 @@ */ public class FeatureFlags { // Prefixes public for testing - public static final String OS_EXPERIMENTAL_PREFIX = "opensearch.experimental."; - public static final String FEATURE_FLAG_PREFIX = OS_EXPERIMENTAL_PREFIX + "feature."; + private static final String OS_EXPERIMENTAL_PREFIX = "opensearch.experimental."; + static final String FEATURE_FLAG_PREFIX = OS_EXPERIMENTAL_PREFIX + "feature."; /** * Gates the visibility of the remote store to docrep migration. From 7b7f9278601cfaf079e7fb75a452069365ef2eed Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Mon, 24 Mar 2025 09:30:25 -0700 Subject: [PATCH 10/10] Nit. Signed-off-by: Finn Carroll --- .../src/main/java/org/opensearch/common/util/FeatureFlags.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index a17d5407dd3b8..8fa914438c1c4 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -247,8 +247,7 @@ private void initFromSettings(Settings settings) { * @return true if feature enabled - else false */ boolean isEnabled(Setting ff) { - if (!featureFlags.containsKey(ff)) return false; - return featureFlags.get(ff); + return featureFlags.getOrDefault(ff, false); } /**