Refactor FeatureFlags #17611
Open
Refactor FeatureFlags #17611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
bug
|
❌ Gradle check result for fdec16b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
fdec16b to
f1d5490
Compare
|
❌ Gradle check result for f1d5490: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
f1d5490 to
1228bda
Compare
|
❌ Gradle check result for 1228bda: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
1228bda to
b688424
Compare
|
❌ Gradle check result for 4a3e60f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Verifying Feature branch: Main branch: |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
4a3e60f to
4bc33cd
Compare
|
❌ Gradle check result for 4bc33cd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
4bc33cd to
94fd7af
Compare
|
❌ Gradle check result for 94fd7af: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
feec9a7 to
e8ad6be
Compare
|
❌ Gradle check result for e8ad6be: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
finnegancarroll
changed the title
|
{"run-benchmark-test": "id_5"} |
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/2624/ . Final results will be published once the job is completed. |
|
{"run-benchmark-test": "id_3"} |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/2624/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/40/
|
This was referenced Mar 19, 2025
Open
|
{"run-benchmark-test": "id_3"} |
finnegancarroll
requested review from
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
bugmakerrrrrr,
jainankitk and
linuxpi
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17611 +/- ##
============================================
- Coverage 72.53% 72.42% -0.12%
+ Complexity 65826 65727 -99
============================================
Files 5311 5311
Lines 305073 305135 +62
Branches 44243 44254 +11
============================================
- Hits 221293 220992 -301
- Misses 65688 66044 +356
- Partials 18092 18099 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
msfroh
reviewed
Mar 20, 2025
msfroh
approved these changes
Mar 20, 2025
- Remove internal immutable `settings` in favor of `ConcurrentHashMap`.
- Move functionality to internal `FeatureFlagsImpl` class.
Expose public api of `FeatureFlagsImpl` in FeatureFlags.
Expose test api of `FeatureFlagsImpl` in FeatureFlags.TestUtils.
- Read and set JVM system properties once on `initializeFeatureFlags`.
Remove JVM system properties check from `isEnabled`.
- Add `FlagLock` in `TestUtils` to maintain a lock for each feature flag.
- Add helper functions to set & access feature flags in a thread safe way.
`TestUtils.with(<feature flag>, () -> {})` to execute crtical sections.
`New FlagLock(<feature flag>)` for fine grained control.
Signed-off-by: Finn Carroll <[email protected]>
- Add annotation in OpenSearchTestCase to enable and lock a flag for the duration of a single test case. Signed-off-by: Finn Carroll <[email protected]>
- Add cases for public api. - Add cases for thread safe helpers @LockFeatureFlag FlagLock TestUtils.with Signed-off-by: Finn Carroll <[email protected]>
Signed-off-by: Finn Carroll <[email protected]>
Replace all usages of `FeatureFlagSetter` in tests. Replace all usages of JVM system properties for feature flags in tests. Replace all usages of `initializeFeatureFlags` with `TestUtils.set` in tests. Signed-off-by: Finn Carroll <[email protected]>
Signed-off-by: Finn Carroll <[email protected]>
- Add missing LockFeatureFlag annotations. - Cannot use annotation in tests which expect exception thrown. - SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY has no setting? Adding. - Flight server tests need flag enabled on setup. Signed-off-by: Finn Carroll <[email protected]>
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
e8ad6be to
10cdbec
Compare
jainankitk
reviewed
Mar 20, 2025
There was a problem hiding this comment.
@finnegancarroll - Few nitpicks, but otherwise this looks great!
Comment on lines
+31
to
+32
| public static final String OS_EXPERIMENTAL_PREFIX = "opensearch.experimental."; | ||
| public static final String FEATURE_FLAG_PREFIX = OS_EXPERIMENTAL_PREFIX + "feature."; |
There was a problem hiding this comment.
nit:
OS_EXPERIMENTAL_PREFIX can be private?
FEATURE_FLAG_PREFIX can be package private for testing?
Comment on lines
+246
to
+247
| if (!featureFlags.containsKey(ff)) return false; | ||
| return featureFlags.get(ff); |
There was a problem hiding this comment.
nit: Could this be return featureFlags.getOrDefault(ff, false)?
Comment on lines
+255
to
+256
| for (Setting<Boolean> ff : featureFlags.keySet()) { | ||
| if (ff.getKey().equals(featureFlagName)) featureFlags.put(ff, value); |
There was a problem hiding this comment.
Initially, I was confused why we need to iterate over the keys for putting featureFlag, then I noticed need to compare the setting name, within Setting object that is the key. I am assuming this is done only during the initialization?
andrross
reviewed
Mar 20, 2025
| } | ||
|
|
||
| /** | ||
| * Provides feature flag write access and synchronization for test use cases. |
There was a problem hiding this comment.
Copying from this comment from @reta about how tests are run:
So this is my understanding of how we do that:
- we run test suites concurrently by forking JVMs
- we never run test suites (or tests with same suite) concurrently within same JVM
The static state between tests should not be shared between JVMs but it is possible that we don't clean up properly after some test suites so this problem emerges.
As far as I know there should be no contention between tests modifying feature flags in the static state because within one JVM there is only ever one test running at one time. That means the concurrency primitives here (ReentrantLocks) should not be needed. I'd be in favor of not using locks if they are not necessary because it will only add to confusion about how tests are run.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There are a handful of competing ways to set feature flags today.
FeatureFlags.initializeFeatureFlagsTakes a setting object and overwrites
FeatureFlags.settings. Node invokes this to set flags on startup, but it is also used throughout unit tests to dynamically update and clear feature flags. Provides no way to set a single flag without rewriting all settings.System.setProperty:Sets the feature flag as a JVM property. Calls to
FeatureFlags.isEnabled()will checkSystem.getProperty()and see this flag. Allows dynamic setting of individual feature flags.FeatureFlagSetter.set:A small wrapper around the above
System.setProperty()which additionally tracks which flags it sets. Used only in tests. Tracks set properties and exposesclearAll()to unset all tracked properties which is invoked duringOpenSearchTestCaseteardown to ensure a "clean slate" for each test.Together these implementations provide the desired functionality for feature flags across test and non-test use cases but present some issues:
System.getProperty()can be slow and have performance implications when feature flags are retrieved in a hot path.System.setProperty(), clearing or setting any single feature flag resets all flags.This PR refactors
FeatureFlagsto consolidate feature flag functionality and remove usage of JVM system properties in all cases exceptinitializeFeatureFlagswhich is called once by Node on startup.Specific changes include:
FeatureFlagsimmutable internal settings with a map.initializeFeatureFlagsnow reads and sets feature flags from JVM system properties.isEnabledno longer checks JVM system properties to mitigate performance impact.FeatureFlags.TestUtilsfor clarity.@LockFeatureFlagannotation provided as a thread safe way to set a feature flag for the duration of the test case.FeatureFlags.TestUtils.withfor thread safe execution of a critical section with provided feature flag enabled.FeatureFlags.TestUtils.FlagLockfor directly accessing a feature flag's lock.Related Issues
Resolves #16519
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.