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"} |
|
❌ Gradle check result for 1e4e8db: 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? |
|
❌ Gradle check result for 1e4e8db: 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 This is great, can we also add a small example/blurb in TESTING.md on proper usage? |
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
1e4e8db to
8b16ab1
Compare
Good idea. It looks like there's a small feature flag related section in the developer guide. Added a note there. |
|
❌ Gradle check result for 8b16ab1: 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? |
|
ReactorNetty4StreamingStressIT is flaky |
- 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]>
JUnit does not run tests in parallel on the same JVM so these are not necessary. Additionally rename to `FlagWriteLock` for clarity. Signed-off-by: Finn Carroll <[email protected]>
Address ff contant nit. Signed-off-by: Finn Carroll <[email protected]>
Signed-off-by: Finn Carroll <[email protected]>
Signed-off-by: Finn Carroll <[email protected]>
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
8b16ab1 to
d52c8c8
Compare
|
❌ Gradle check result for d52c8c8: 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? |
|
❌ Gradle check result for d52c8c8: 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? |
|
❌ Gradle check result for d52c8c8: 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? |
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 to set the value of a feature flag and make it immutable for the duration of the test case.FeatureFlags.TestUtils.withfor execution of a single runnable with feature flag enabled.FeatureFlags.TestUtils.FlagWriteLockfor directly accessing a feature flag's write 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.