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 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..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 @@ -28,7 +28,6 @@ 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 +54,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; @@ -64,6 +64,7 @@ @SuppressWarnings("unchecked") public class FlightClientManagerTests extends OpenSearchTestCase { + private static FeatureFlags.TestUtils.FlagWriteLock ffLock = null; private static BufferAllocator allocator; private static EventLoopGroup elg; @@ -78,6 +79,7 @@ public class FlightClientManagerTests extends OpenSearchTestCase { @BeforeClass public static void setupClass() throws Exception { + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(ARROW_STREAMS); ServerConfig.init(Settings.EMPTY); allocator = new RootAllocator(); elg = ServerConfig.createELG("test-grpc-worker-elg", NettyRuntime.availableProcessors() * 2); @@ -89,7 +91,6 @@ 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(); @@ -176,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/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..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 @@ -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.FlagWriteLock 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.FlagWriteLock(ARROW_STREAMS); int availablePort = getBasePort(9500) + port.addAndGet(1); settings = Settings.EMPTY; localNode = createNode(availablePort); @@ -147,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 4ff81cf0c1c96..8fa914438c1c4 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,57 @@ import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; -import java.util.List; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; /** - * 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 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 + 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. */ - 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"; + public static final Setting SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_SETTING = Setting.boolSetting( + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, + false, + Property.NodeScope + ); /** * 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 +73,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 +109,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 +125,243 @@ 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)); + put( + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_SETTING, + SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_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 write locked according to TestUtils.FlagLock. + */ + private void initFromDefaults() { + for (Setting ff : featureFlags.keySet()) { + if (TestUtils.FlagWriteLock.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 write locked according to TestUtils.FlagLock. + */ + private void initFromSysProperties() { + for (Setting ff : featureFlags.keySet()) { + if (TestUtils.FlagWriteLock.isLocked(ff.getKey())) continue; + String prop = System.getProperty(ff.getKey()); + if (prop != null) { + featureFlags.put(ff, Boolean.valueOf(prop)); + } + } + } + + /** + * 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 + */ + private void initFromSettings(Settings settings) { + for (Setting ff : featureFlags.keySet()) { + if (settings.hasValue(ff.getKey())) { + if (TestUtils.FlagWriteLock.isLocked(ff.getKey())) continue; + featureFlags.put(ff, settings.getAsBoolean(ff.getKey(), ff.getDefault(settings))); + } + } + } + + /** + * @param ff feature flag setting + * @return true if feature enabled - else false + */ + boolean isEnabled(Setting ff) { + return featureFlags.getOrDefault(ff, false); + } + + /** + * @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 featureFlagName feature flag 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); + } } - 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 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 { + /** + * 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 FlagWriteLock implements AutoCloseable { + private static final Set writeLocks = new HashSet<>(); + private final String flag; + private final Boolean prev; + + public static boolean isLocked(String flag) { + return writeLocks.contains(flag); + } + + public FlagWriteLock(String flag) { + this(flag, true); + } + + public FlagWriteLock(String flag, Boolean value) { + if (writeLocks.contains(flag)) { + throw new RuntimeException("Cannot initialize second write lock for feature flag: " + flag); + } + this.flag = flag; + this.prev = featureFlagsImpl.isEnabled(flag); + writeLocks.add(flag); + featureFlagsImpl.set(flag, value); + } + + public void unlock() { + featureFlagsImpl.set(flag, prev); + writeLocks.remove(flag); + } + + @Override + public void close() { + featureFlagsImpl.set(flag, prev); + writeLocks.remove(flag); + } + } + + /** + * For critical sections run as lambdas which may throw exceptions. + */ + @FunctionalInterface + public interface ThrowingRunnable { + void run() throws Exception; + } + + /** + * 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 flag, ThrowingRunnable action) throws Exception { + try (FlagWriteLock ignored = new FlagWriteLock(flag)) { + action.run(); + } + } + + 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/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..a91e724ec279d 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( @@ -972,16 +972,18 @@ 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"); } + @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( @@ -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); @@ -2629,8 +2633,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 +2755,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/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/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/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index 6d9d1aad3c5d5..f3751e98f5b60 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,121 @@ 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 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 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)); } - public void testBooleanFeatureFlagWithDefaultSetToFalse() { - final String testFlag = EXTENSIONS; - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); - assertNotNull(testFlag); - assertFalse(FeatureFlags.isEnabled(testFlag)); + /** + * Test global feature flag instance. + */ + + public void testLockFeatureFlagWithFlagLock() { + 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 + } } - public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToFalse() { - final String testFlag = DATETIME_FORMATTER_CACHING; - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); - assertFalse(FeatureFlags.isEnabled(testFlag)); + 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 + }); } - 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 testLockFeatureFlagAnnotation() { + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked } } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 8b0a455353c5f..df03120d06fb0 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.FlagWriteLock 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.FlagWriteLock(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..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,6 +51,7 @@ */ @LuceneTestCase.SuppressSysoutChecks(bugUrl = "we log a lot on purpose") public abstract class AbstractStarTreeDVFormatTests extends BaseDocValuesFormatTestCase { + private static FeatureFlags.TestUtils.FlagWriteLock 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.FlagWriteLock(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..8b2eb67481abf 100644 --- a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java @@ -556,23 +556,20 @@ 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)); - - 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.initializeFeatureFlags(Settings.EMPTY); + 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/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java index 435621548942b..b355aaea24121 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.FlagWriteLock ffLock = null; @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + ffLock = new FeatureFlags.TestUtils.FlagWriteLock(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..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 @@ -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.FlagWriteLock 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.FlagWriteLock(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..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 @@ -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.FlagWriteLock 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.FlagWriteLock(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..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 @@ -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.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() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + fflock = new FeatureFlags.TestUtils.FlagWriteLock(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..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 @@ -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.FlagWriteLock 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.FlagWriteLock(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..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 @@ -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.FlagWriteLock 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.FlagWriteLock(STAR_TREE_INDEX); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + fflock.close(); } protected Codec getCodec(int maxLeafDoc, boolean skipStarNodeCreationForSDVDimension) { 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(); - } -} 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..2525aef903298 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,45 @@ public abstract class OpenSearchTestCase extends LuceneTestCase { private static final Collection nettyLoggedLeaks = new ArrayList<>(); private HeaderWarningAppender headerWarningAppender; + /** + * Define LockFeatureFlag annotation for unit tests. + * 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) + @Target({ ElementType.METHOD }) + public @interface LockFeatureFlag { + String value(); + } + + public static class AnnotatedFeatureFlagRule implements TestRule { + /** + * Wrap base test case with an + * @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.FlagWriteLock ignored = new FeatureFlags.TestUtils.FlagWriteLock(flagKey)) { + base.evaluate(); + } + } + }; + } + } + + @Rule + public AnnotatedFeatureFlagRule flagLockRule = new AnnotatedFeatureFlagRule(); + @AfterClass public static void resetPortCounter() { portGenerator.set(0); @@ -242,7 +289,6 @@ public static void resetPortCounter() { @Override public void tearDown() throws Exception { Schedulers.shutdownNow(); - FeatureFlagSetter.clear(); super.tearDown(); } @@ -1233,7 +1279,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 +1441,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 +1733,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() ); }