Refactor FeatureFlags #17611
Open
Refactor FeatureFlags #17611
+528
−339
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
bugmakerrrrrr,
jainankitk and
linuxpi
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17611 +/- ##
============================================
+ Coverage 72.40% 72.43% +0.03%
+ Complexity 65828 65823 -5
============================================
Files 5316 5316
Lines 305294 305395 +101
Branches 44289 44303 +14
============================================
+ Hits 221033 221208 +175
+ Misses 66187 66112 -75
- Partials 18074 18075 +1 ☔ 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
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!
server/src/main/java/org/opensearch/common/util/FeatureFlags.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/util/FeatureFlags.java
Outdated
Show resolved
Hide resolved
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?
There was a problem hiding this comment.
I think Ideally feature flags would be represented as a Setting everywhere. However, since annotations only take primitive types I ended up with this pattern where I look up the feature flag from its setting key.
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.
There was a problem hiding this comment.
I see. In hindsight that makes sense as otherwise OpenSearchTestCase resetting all flags every test would make feature flags extremely unstable. I notice that if I remove the FlagLock class completely I run into issues as test cases sometimes initialize new nodes which clears all previous feature flag settings. This breaks the annotation since it sets the flag before test setup.
I've moved the FlagLock class to just make the flag immutable for its lifetime to preserve the annotation functionality. Let me know if this makes sense or if it would be more appropriate to drop the annotation & FlagLock class entirely.
|
❌ Gradle check result for af94175: 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? |
- 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 <carrofin@amazon.com>
- Add annotation in OpenSearchTestCase to enable and lock a flag for the duration of a single test case. Signed-off-by: Finn Carroll <carrofin@amazon.com>
- Add cases for public api. - Add cases for thread safe helpers @LockFeatureFlag FlagLock TestUtils.with Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
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 <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
- 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 <carrofin@amazon.com>
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 <carrofin@amazon.com>
Address ff contant nit. Signed-off-by: Finn Carroll <carrofin@amazon.com>
finnegancarroll
force-pushed
the
feature-flag-kinda-slow
branch
from
1bf0ba1 to
9a35bf1
Compare
|
❌ Gradle check result for 7b7f927: 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? |
|
ReactorNetty4StreamingStressIT is flaky (reached timeout) |
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.