Skip to content

Commit f96669a

Browse files
cwperkscwperks
authored
Fix issue with feature flags where default value may not be honored (opensearch-project#12849) (opensearch-project#12912)
* Fix issue with feature flags passed as system props where default value was not being honored Signed-off-by: Craig Perkins <craig5008@gmail.com> * Add CHANGELOG entry Signed-off-by: Craig Perkins <craig5008@gmail.com> * Add test for default value of false Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix issue when empty settings passed in initialization Signed-off-by: Craig Perkins <cwperx@amazon.com> * Get actual value from settings and default from ff setting Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add test with non-empty setting initialization Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <craig5008@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> (cherry picked from commit 12f1487)
1 parent 3418137 commit f96669a

File tree

3 files changed

+89
-28
lines changed

3 files changed

+89
-28
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2121
### Removed
2222

2323
### Fixed
24+
- Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849))
2425

2526
### Security
2627

‎server/src/main/java/org/opensearch/common/util/FeatureFlags.java

+54-28
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@
1212
import org.opensearch.common.settings.Setting.Property;
1313
import org.opensearch.common.settings.Settings;
1414

15+
import java.util.List;
16+
1517
/**
1618
* Utility class to manage feature flags. Feature flags are system properties that must be set on the JVM.
17-
* These are used to gate the visibility/availability of incomplete features. Fore more information, see
19+
* These are used to gate the visibility/availability of incomplete features. For more information, see
1820
* https://featureflags.io/feature-flag-introduction/
1921
*
2022
* @opensearch.internal
@@ -65,19 +67,69 @@ public class FeatureFlags {
6567
*/
6668
public static final String PLUGGABLE_CACHE = "opensearch.experimental.feature.pluggable.caching.enabled";
6769

70+
public static final Setting<Boolean> REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
71+
REMOTE_STORE_MIGRATION_EXPERIMENTAL,
72+
false,
73+
Property.NodeScope
74+
);
75+
76+
public static final Setting<Boolean> EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope);
77+
78+
public static final Setting<Boolean> IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope);
79+
80+
public static final Setting<Boolean> TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope);
81+
82+
public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
83+
DATETIME_FORMATTER_CACHING,
84+
true,
85+
Property.NodeScope
86+
);
87+
88+
public static final Setting<Boolean> WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting(
89+
WRITEABLE_REMOTE_INDEX,
90+
false,
91+
Property.NodeScope
92+
);
93+
94+
public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);
95+
96+
private static final List<Setting<Boolean>> ALL_FEATURE_FLAG_SETTINGS = List.of(
97+
REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING,
98+
EXTENSIONS_SETTING,
99+
IDENTITY_SETTING,
100+
TELEMETRY_SETTING,
101+
DATETIME_FORMATTER_CACHING_SETTING,
102+
WRITEABLE_REMOTE_INDEX_SETTING,
103+
PLUGGABLE_CACHE_SETTING
104+
);
68105
/**
69106
* Should store the settings from opensearch.yml.
70107
*/
71108
private static Settings settings;
72109

110+
static {
111+
Settings.Builder settingsBuilder = Settings.builder();
112+
for (Setting<Boolean> ffSetting : ALL_FEATURE_FLAG_SETTINGS) {
113+
settingsBuilder = settingsBuilder.put(ffSetting.getKey(), ffSetting.getDefault(Settings.EMPTY));
114+
}
115+
settings = settingsBuilder.build();
116+
}
117+
73118
/**
74119
* This method is responsible to map settings from opensearch.yml to local stored
75120
* settings value. That is used for the existing isEnabled method.
76121
*
77122
* @param openSearchSettings The settings stored in opensearch.yml.
78123
*/
79124
public static void initializeFeatureFlags(Settings openSearchSettings) {
80-
settings = openSearchSettings;
125+
Settings.Builder settingsBuilder = Settings.builder();
126+
for (Setting<Boolean> ffSetting : ALL_FEATURE_FLAG_SETTINGS) {
127+
settingsBuilder = settingsBuilder.put(
128+
ffSetting.getKey(),
129+
openSearchSettings.getAsBoolean(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings))
130+
);
131+
}
132+
settings = settingsBuilder.build();
81133
}
82134

83135
/**
@@ -103,30 +155,4 @@ public static boolean isEnabled(Setting<Boolean> featureFlag) {
103155
return featureFlag.getDefault(Settings.EMPTY);
104156
}
105157
}
106-
107-
public static final Setting<Boolean> REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
108-
REMOTE_STORE_MIGRATION_EXPERIMENTAL,
109-
false,
110-
Property.NodeScope
111-
);
112-
113-
public static final Setting<Boolean> EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope);
114-
115-
public static final Setting<Boolean> IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope);
116-
117-
public static final Setting<Boolean> TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope);
118-
119-
public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
120-
DATETIME_FORMATTER_CACHING,
121-
true,
122-
Property.NodeScope
123-
);
124-
125-
public static final Setting<Boolean> WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting(
126-
WRITEABLE_REMOTE_INDEX,
127-
false,
128-
Property.NodeScope
129-
);
130-
131-
public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);
132158
}

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

+34
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,14 @@
88

99
package org.opensearch.common.util;
1010

11+
import org.opensearch.common.settings.Settings;
1112
import org.opensearch.test.FeatureFlagSetter;
1213
import org.opensearch.test.OpenSearchTestCase;
1314

15+
import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING;
16+
import static org.opensearch.common.util.FeatureFlags.EXTENSIONS;
17+
import static org.opensearch.common.util.FeatureFlags.IDENTITY;
18+
1419
public class FeatureFlagTests extends OpenSearchTestCase {
1520

1621
private final String FLAG_PREFIX = "opensearch.experimental.feature.";
@@ -33,4 +38,33 @@ public void testNonBooleanFeatureFlag() {
3338
assertNotNull(System.getProperty(javaVersionProperty));
3439
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
3540
}
41+
42+
public void testBooleanFeatureFlagWithDefaultSetToTrue() {
43+
final String testFlag = DATETIME_FORMATTER_CACHING;
44+
assertNotNull(testFlag);
45+
assertTrue(FeatureFlags.isEnabled(testFlag));
46+
}
47+
48+
public void testBooleanFeatureFlagWithDefaultSetToFalse() {
49+
final String testFlag = IDENTITY;
50+
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
51+
assertNotNull(testFlag);
52+
assertFalse(FeatureFlags.isEnabled(testFlag));
53+
}
54+
55+
public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToTrue() {
56+
final String testFlag = DATETIME_FORMATTER_CACHING;
57+
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
58+
assertNotNull(testFlag);
59+
assertTrue(FeatureFlags.isEnabled(testFlag));
60+
}
61+
62+
public void testInitializeFeatureFlagsWithExperimentalSettings() {
63+
FeatureFlags.initializeFeatureFlags(Settings.builder().put(IDENTITY, true).build());
64+
assertTrue(FeatureFlags.isEnabled(IDENTITY));
65+
assertTrue(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING));
66+
assertFalse(FeatureFlags.isEnabled(EXTENSIONS));
67+
// reset FeatureFlags to defaults
68+
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
69+
}
3670
}

0 commit comments

Comments
 (0)