Skip to content

Commit b95e49c

Browse files
kartgkartg
committed
Added FeatureFlagSetter helper class
Also refactored unit test classes to use the helper class. Signed-off-by: Kartik Ganesh <[email protected]>
1 parent 1ab2481 commit b95e49c

File tree

6 files changed

+98
-82
lines changed

6 files changed

+98
-82
lines changed

‎server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java‎

Lines changed: 38 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,9 @@
3434

3535
import org.opensearch.common.inject.ModuleTestCase;
3636
import org.opensearch.common.settings.Setting.Property;
37-
import org.opensearch.common.util.FeatureFlagTests;
3837
import org.hamcrest.Matchers;
38+
import org.opensearch.common.util.FeatureFlags;
39+
import org.opensearch.test.FeatureFlagSetter;
3940

4041
import java.util.Arrays;
4142

@@ -239,46 +240,48 @@ public void testOldMaxClauseCountSetting() {
239240
);
240241
}
241242

242-
public void testDynamicNodeSettingsRegistration() {
243-
FeatureFlagTests.enableFeature();
244-
Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
245-
SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
246-
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
247-
// For unregistered setting the value is expected to be null
248-
assertNull(module.getClusterSettings().get("some.custom.setting2"));
249-
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
243+
public void testDynamicNodeSettingsRegistration() throws Exception {
244+
try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) {
245+
Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
246+
SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
247+
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
248+
// For unregistered setting the value is expected to be null
249+
assertNull(module.getClusterSettings().get("some.custom.setting2"));
250+
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
250251

251-
assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope)));
252-
assertNotNull(module.getClusterSettings().get("some.custom.setting2"));
253-
// verify if some.custom.setting still exists
254-
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
252+
assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope)));
253+
assertNotNull(module.getClusterSettings().get("some.custom.setting2"));
254+
// verify if some.custom.setting still exists
255+
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
255256

256-
// verify exception is thrown when setting registration fails
257-
expectThrows(
258-
SettingsException.class,
259-
() -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope))
260-
);
257+
// verify exception is thrown when setting registration fails
258+
expectThrows(
259+
SettingsException.class,
260+
() -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope))
261+
);
262+
}
261263
}
262264

263-
public void testDynamicIndexSettingsRegistration() {
264-
FeatureFlagTests.enableFeature();
265-
Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
266-
SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
267-
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
268-
// For unregistered setting the value is expected to be null
269-
assertNull(module.getIndexScopedSettings().get("index.custom.setting2"));
270-
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
265+
public void testDynamicIndexSettingsRegistration() throws Exception {
266+
try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS)) {
267+
Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
268+
SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
269+
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
270+
// For unregistered setting the value is expected to be null
271+
assertNull(module.getIndexScopedSettings().get("index.custom.setting2"));
272+
assertInstanceBinding(module, Settings.class, (s) -> s == settings);
271273

272-
assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)));
273-
assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2"));
274+
assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)));
275+
assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2"));
274276

275-
// verify if some.custom.setting still exists
276-
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
277+
// verify if some.custom.setting still exists
278+
assertNotNull(module.getClusterSettings().get("some.custom.setting"));
277279

278-
// verify exception is thrown when setting registration fails
279-
expectThrows(
280-
SettingsException.class,
281-
() -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))
282-
);
280+
// verify exception is thrown when setting registration fails
281+
expectThrows(
282+
SettingsException.class,
283+
() -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))
284+
);
285+
}
283286
}
284287
}

‎server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java‎

Lines changed: 9 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,31 +8,23 @@
88

99
package org.opensearch.common.util;
1010

11-
import org.junit.BeforeClass;
12-
import org.opensearch.common.SuppressForbidden;
11+
import org.opensearch.test.FeatureFlagSetter;
1312
import org.opensearch.test.OpenSearchTestCase;
1413

15-
import java.security.AccessController;
16-
import java.security.PrivilegedAction;
17-
1814
public class FeatureFlagTests extends OpenSearchTestCase {
1915

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

28-
public void testReplicationTypeFeatureFlag() {
29-
String replicationTypeFlag = FeatureFlags.REPLICATION_TYPE;
30-
assertNotNull(System.getProperty(replicationTypeFlag));
31-
assertTrue(FeatureFlags.isEnabled(replicationTypeFlag));
18+
public void testFeatureFlagSet() throws Exception {
19+
final String testFlag = FLAG_PREFIX + "testFlag";
20+
try (FeatureFlagSetter f = FeatureFlagSetter.set(testFlag)) {
21+
assertNotNull(System.getProperty(testFlag));
22+
assertTrue(FeatureFlags.isEnabled(testFlag));
23+
}
3224
}
3325

3426
public void testMissingFeatureFlag() {
35-
String testFlag = "missingFeatureFlag";
27+
final String testFlag = FLAG_PREFIX + "testFlag";
3628
assertNull(System.getProperty(testFlag));
3729
assertFalse(FeatureFlags.isEnabled(testFlag));
3830
}
@@ -42,11 +34,4 @@ public void testNonBooleanFeatureFlag() {
4234
assertNotNull(System.getProperty(javaVersionProperty));
4335
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
4436
}
45-
46-
public void testRemoteStoreFeatureFlag() {
47-
String remoteStoreFlag = FeatureFlags.REMOTE_STORE;
48-
assertNotNull(System.getProperty(remoteStoreFlag));
49-
assertTrue(FeatureFlags.isEnabled(remoteStoreFlag));
50-
}
51-
5237
}

‎server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
import org.opensearch.common.network.NetworkService;
5656
import org.opensearch.common.settings.Settings;
5757
import org.opensearch.common.transport.TransportAddress;
58-
import org.opensearch.common.util.FeatureFlagTests;
58+
import org.opensearch.common.util.FeatureFlags;
5959
import org.opensearch.common.util.PageCacheRecycler;
6060
import org.opensearch.common.util.concurrent.ThreadContext;
6161
import org.opensearch.env.Environment;
@@ -70,6 +70,7 @@
7070
import org.opensearch.indices.breaker.NoneCircuitBreakerService;
7171
import org.opensearch.plugins.PluginInfo;
7272
import org.opensearch.rest.RestController;
73+
import org.opensearch.test.FeatureFlagSetter;
7374
import org.opensearch.test.IndexSettingsModule;
7475
import org.opensearch.test.MockLogAppender;
7576
import org.opensearch.test.OpenSearchTestCase;
@@ -84,6 +85,7 @@
8485

8586
public class ExtensionsManagerTests extends OpenSearchTestCase {
8687

88+
private FeatureFlagSetter featureFlagSetter;
8789
private TransportService transportService;
8890
private RestController restController;
8991
private ClusterService clusterService;
@@ -124,7 +126,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase {
124126

125127
@Before
126128
public void setup() throws Exception {
127-
FeatureFlagTests.enableFeature();
129+
featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS);
128130
Settings settings = Settings.builder().put("cluster.name", "test").build();
129131
transport = new MockNioTransport(
130132
settings,
@@ -169,6 +171,7 @@ public void tearDown() throws Exception {
169171
super.tearDown();
170172
transportService.close();
171173
ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS);
174+
featureFlagSetter.close();
172175
}
173176

174177
public void testDiscover() throws Exception {

‎server/src/test/java/org/opensearch/index/IndexSettingsTests.java‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,10 @@
4545
import org.opensearch.common.util.FeatureFlags;
4646
import org.opensearch.index.translog.Translog;
4747
import org.opensearch.indices.replication.common.ReplicationType;
48+
import org.opensearch.test.FeatureFlagSetter;
4849
import org.opensearch.test.OpenSearchTestCase;
4950
import org.opensearch.test.VersionUtils;
5051

51-
import java.security.AccessController;
52-
import java.security.PrivilegedAction;
5352
import java.util.Arrays;
5453
import java.util.Collections;
5554
import java.util.HashSet;
@@ -950,11 +949,8 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() {
950949
}
951950

952951
@SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag")
953-
public void testExtendedCompatibilityVersionForRemoteSnapshot() {
954-
try {
955-
AccessController.doPrivileged(
956-
(PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true")
957-
);
952+
public void testExtendedCompatibilityVersionForRemoteSnapshot() throws Exception {
953+
try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
958954
IndexMetadata metadata = newIndexMeta(
959955
"index",
960956
Settings.builder()
@@ -965,10 +961,6 @@ public void testExtendedCompatibilityVersionForRemoteSnapshot() {
965961
IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY);
966962
assertTrue(settings.isRemoteSnapshot());
967963
assertEquals(SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION, settings.getExtendedCompatibilitySnapshotVersion());
968-
} finally {
969-
AccessController.doPrivileged(
970-
(PrivilegedAction<String>) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)
971-
);
972964
}
973965
}
974966

‎server/src/test/java/org/opensearch/index/store/StoreTests.java‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
import org.opensearch.indices.replication.common.ReplicationType;
8888
import org.opensearch.indices.store.TransportNodesListShardStoreMetadata;
8989
import org.opensearch.test.DummyShardLock;
90+
import org.opensearch.test.FeatureFlagSetter;
9091
import org.opensearch.test.IndexSettingsModule;
9192
import org.opensearch.test.OpenSearchTestCase;
9293

@@ -96,8 +97,6 @@
9697
import java.io.IOException;
9798
import java.nio.file.NoSuchFileException;
9899
import java.nio.file.Path;
99-
import java.security.AccessController;
100-
import java.security.PrivilegedAction;
101100
import java.util.ArrayList;
102101
import java.util.Arrays;
103102
import java.util.Collections;
@@ -1264,17 +1263,15 @@ public void testSegmentReplicationDiff() {
12641263
}
12651264

12661265
@SuppressForbidden(reason = "sets the SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY feature flag")
1267-
public void testReadSegmentsFromOldIndices() throws IOException {
1266+
public void testReadSegmentsFromOldIndices() throws Exception {
12681267
int expectedIndexCreatedVersionMajor = SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION.luceneVersion.major;
12691268
final String pathToTestIndex = "/indices/bwc/es-6.3.0/testIndex-es-6.3.0.zip";
12701269
Path tmp = createTempDir();
12711270
TestUtil.unzip(getClass().getResourceAsStream(pathToTestIndex), tmp);
12721271
final ShardId shardId = new ShardId("index", "_na_", 1);
12731272
Store store = null;
1274-
try {
1275-
AccessController.doPrivileged(
1276-
(PrivilegedAction<String>) () -> System.setProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY, "true")
1277-
);
1273+
1274+
try (FeatureFlagSetter f = FeatureFlagSetter.set(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)) {
12781275
IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
12791276
"index",
12801277
Settings.builder()
@@ -1285,9 +1282,6 @@ public void testReadSegmentsFromOldIndices() throws IOException {
12851282
store = new Store(shardId, indexSettings, StoreTests.newMockFSDirectory(tmp), new DummyShardLock(shardId));
12861283
assertEquals(expectedIndexCreatedVersionMajor, store.readLastCommittedSegmentsInfo().getIndexCreatedVersionMajor());
12871284
} finally {
1288-
AccessController.doPrivileged(
1289-
(PrivilegedAction<String>) () -> System.clearProperty(FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY)
1290-
);
12911285
if (store != null) {
12921286
store.close();
12931287
}

‎test/framework/src/main/java/org/opensearch/test/FeatureFlagSetter.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.test;
10+
11+
import org.opensearch.common.SuppressForbidden;
12+
13+
import java.security.AccessController;
14+
import java.security.PrivilegedAction;
15+
16+
/**
17+
* Helper class that wraps the lifecycle of setting and finally clearing of
18+
* a {@link org.opensearch.common.util.FeatureFlags} string in an {@link AutoCloseable}.
19+
*/
20+
public class FeatureFlagSetter implements AutoCloseable {
21+
22+
private final String flag;
23+
24+
private FeatureFlagSetter(String flag) {
25+
this.flag = flag;
26+
}
27+
28+
@SuppressForbidden(reason = "Enables setting of feature flags")
29+
public static final FeatureFlagSetter set(String flag) {
30+
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.setProperty(flag, "true"));
31+
return new FeatureFlagSetter(flag);
32+
}
33+
34+
@SuppressForbidden(reason = "Clears the set feature flag on close")
35+
@Override
36+
public void close() throws Exception {
37+
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.clearProperty(this.flag));
38+
}
39+
}

0 commit comments

Comments
 (0)