Skip to content

Commit

Permalink
Added FeatureFlagSetter helper class
Browse files Browse the repository at this point in the history 
Also refactored unit test classes to use the helper class.

Signed-off-by: Kartik Ganesh <gkart@amazon.com>
  • Loading branch information
@kartg
kartg committed Dec 24, 2022
1 parent 1ab2481 commit b95e49c
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 82 deletions.
73 changes: 38 additions & 35 deletions server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@

import org.opensearch.common.inject.ModuleTestCase;
import org.opensearch.common.settings.Setting.Property;
import org.opensearch.common.util.FeatureFlagTests;
import org.hamcrest.Matchers;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.test.FeatureFlagSetter;

import java.util.Arrays;

Expand Down Expand Up @@ -239,46 +240,48 @@ public void testOldMaxClauseCountSetting() {
);
}

public void testDynamicNodeSettingsRegistration() {
FeatureFlagTests.enableFeature();
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"));
// For unregistered setting the value is expected to be null
assertNull(module.getClusterSettings().get("some.custom.setting2"));
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
public void testDynamicNodeSettingsRegistration() throws Exception {
try (FeatureFlagSetter f = 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"));
// For unregistered setting the value is expected to be null
assertNull(module.getClusterSettings().get("some.custom.setting2"));
assertInstanceBinding(module, Settings.class, (s) -> s == settings);

assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope)));
assertNotNull(module.getClusterSettings().get("some.custom.setting2"));
// verify if some.custom.setting still exists
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope)));
assertNotNull(module.getClusterSettings().get("some.custom.setting2"));
// verify if some.custom.setting still exists
assertNotNull(module.getClusterSettings().get("some.custom.setting"));

// verify exception is thrown when setting registration fails
expectThrows(
SettingsException.class,
() -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope))
);
// verify exception is thrown when setting registration fails
expectThrows(
SettingsException.class,
() -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope))
);
}
}

public void testDynamicIndexSettingsRegistration() {
FeatureFlagTests.enableFeature();
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"));
// For unregistered setting the value is expected to be null
assertNull(module.getIndexScopedSettings().get("index.custom.setting2"));
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
public void testDynamicIndexSettingsRegistration() throws Exception {
try (FeatureFlagSetter f = 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"));
// For unregistered setting the value is expected to be null
assertNull(module.getIndexScopedSettings().get("index.custom.setting2"));
assertInstanceBinding(module, Settings.class, (s) -> s == settings);

assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)));
assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2"));
assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)));
assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2"));

// verify if some.custom.setting still exists
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
// verify if some.custom.setting still exists
assertNotNull(module.getClusterSettings().get("some.custom.setting"));

// verify exception is thrown when setting registration fails
expectThrows(
SettingsException.class,
() -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))
);
// verify exception is thrown when setting registration fails
expectThrows(
SettingsException.class,
() -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))
);
}
}
}
33 changes: 9 additions & 24 deletions server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,23 @@

package org.opensearch.common.util;

import org.junit.BeforeClass;
import org.opensearch.common.SuppressForbidden;
import org.opensearch.test.FeatureFlagSetter;
import org.opensearch.test.OpenSearchTestCase;

import java.security.AccessController;
import java.security.PrivilegedAction;

public class FeatureFlagTests extends OpenSearchTestCase {

@SuppressForbidden(reason = "sets the feature flag")
@BeforeClass
public static void enableFeature() {
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.REPLICATION_TYPE, "true"));
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.REMOTE_STORE, "true"));
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.EXTENSIONS, "true"));
}
private final String FLAG_PREFIX = "opensearch.experimental.feature.";

public void testReplicationTypeFeatureFlag() {
String replicationTypeFlag = FeatureFlags.REPLICATION_TYPE;
assertNotNull(System.getProperty(replicationTypeFlag));
assertTrue(FeatureFlags.isEnabled(replicationTypeFlag));
public void testFeatureFlagSet() throws Exception {
final String testFlag = FLAG_PREFIX + "testFlag";
try (FeatureFlagSetter f = FeatureFlagSetter.set(testFlag)) {
assertNotNull(System.getProperty(testFlag));
assertTrue(FeatureFlags.isEnabled(testFlag));
}
}

public void testMissingFeatureFlag() {
String testFlag = "missingFeatureFlag";
final String testFlag = FLAG_PREFIX + "testFlag";
assertNull(System.getProperty(testFlag));
assertFalse(FeatureFlags.isEnabled(testFlag));
}
Expand All @@ -42,11 +34,4 @@ public void testNonBooleanFeatureFlag() {
assertNotNull(System.getProperty(javaVersionProperty));
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
}

public void testRemoteStoreFeatureFlag() {
String remoteStoreFlag = FeatureFlags.REMOTE_STORE;
assertNotNull(System.getProperty(remoteStoreFlag));
assertTrue(FeatureFlags.isEnabled(remoteStoreFlag));
}

}
7 changes: 5 additions & 2 deletions server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
import org.opensearch.common.network.NetworkService;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.FeatureFlagTests;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.common.util.PageCacheRecycler;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.env.Environment;
Expand All @@ -70,6 +70,7 @@
import org.opensearch.indices.breaker.NoneCircuitBreakerService;
import org.opensearch.plugins.PluginInfo;
import org.opensearch.rest.RestController;
import org.opensearch.test.FeatureFlagSetter;
import org.opensearch.test.IndexSettingsModule;
import org.opensearch.test.MockLogAppender;
import org.opensearch.test.OpenSearchTestCase;
Expand All @@ -84,6 +85,7 @@

public class ExtensionsManagerTests extends OpenSearchTestCase {

private FeatureFlagSetter featureFlagSetter;
private TransportService transportService;
private RestController restController;
private ClusterService clusterService;
Expand Down Expand Up @@ -124,7 +126,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase {

@Before
public void setup() throws Exception {
FeatureFlagTests.enableFeature();
featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS);
Settings settings = Settings.builder().put("cluster.name", "test").build();
transport = new MockNioTransport(
settings,
Expand Down Expand Up @@ -169,6 +171,7 @@ public void tearDown() throws Exception {
super.tearDown();
transportService.close();
ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS);
featureFlagSetter.close();
}

public void testDiscover() throws Exception {
Expand Down
14 changes: 3 additions & 11 deletions server/src/test/java/org/opensearch/index/IndexSettingsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,10 @@
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.index.translog.Translog;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.test.FeatureFlagSetter;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.VersionUtils;

import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
Expand Down Expand Up @@ -950,11 +949,8 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() {
}

@SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag")
public void testExtendedCompatibilityVersionForRemoteSnapshot() {
try {
AccessController.doPrivileged(
(PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true")
);
public void testExtendedCompatibilityVersionForRemoteSnapshot() throws Exception {
try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
IndexMetadata metadata = newIndexMeta(
"index",
Settings.builder()
Expand All @@ -965,10 +961,6 @@ public void testExtendedCompatibilityVersionForRemoteSnapshot() {
IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY);
assertTrue(settings.isRemoteSnapshot());
assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion());
} finally {
AccessController.doPrivileged(
(PrivilegedAction<String>) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)
);
}
}

Expand Down
14 changes: 4 additions & 10 deletions server/src/test/java/org/opensearch/index/store/StoreTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.indices.store.TransportNodesListShardStoreMetadata;
import org.opensearch.test.DummyShardLock;
import org.opensearch.test.FeatureFlagSetter;
import org.opensearch.test.IndexSettingsModule;
import org.opensearch.test.OpenSearchTestCase;

Expand All @@ -96,8 +97,6 @@
import java.io.IOException;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -1264,17 +1263,15 @@ public void testSegmentReplicationDiff() {
}

@SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag")
public void testReadSegmentsFromOldIndices() throws IOException {
public void testReadSegmentsFromOldIndices() throws Exception {
int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major;
final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip";
Path tmp = createTempDir();
TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp);
final ShardId shardId = new ShardId("index", "_na_", 1);
Store store = null;
try {
AccessController.doPrivileged(
(PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true")
);

try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
"index",
Settings.builder()
Expand All @@ -1285,9 +1282,6 @@ public void testReadSegmentsFromOldIndices() throws IOException {
store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId));
assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor());
} finally {
AccessController.doPrivileged(
(PrivilegedAction<String>) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)
);
if (store != null) {
store.close();
}
Expand Down
39 changes: 39 additions & 0 deletions test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* 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.opensearch.common.SuppressForbidden;

import java.security.AccessController;
import java.security.PrivilegedAction;

/**
* Helper class that wraps the lifecycle of setting and finally clearing of
* a {@link org.opensearch.common.util.FeatureFlags} string in an {@link AutoCloseable}.
*/
public class FeatureFlagSetter implements AutoCloseable {

private final String flag;

private FeatureFlagSetter(String flag) {
this.flag = flag;
}

@SuppressForbidden(reason = "Enables setting of feature flags")
public static final FeatureFlagSetter set(String flag) {
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(flag, "true"));
return new FeatureFlagSetter(flag);
}

@SuppressForbidden(reason = "Clears the set feature flag on close")
@Override
public void close() throws Exception {
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.clearProperty(this.flag));
}
}

0 comments on commit b95e49c

Please sign in to comment.