Conversation
c46ab20 to
8c7ccf6
Compare
coszio
approved these changes
Mar 18, 2025
Contributor
coszio
left a comment
There was a problem hiding this comment.
Other than figuring out if the CI error is related, I agree this is a good way to:
- keep temporary flags in one place
- not having to propagate them in the code
- being able to change them without recompiling
JojiiOfficial
approved these changes
Mar 19, 2025
Contributor
JojiiOfficial
left a comment
There was a problem hiding this comment.
I like this idea!
I created some minor suggestions in #6198 but they're not necessary.
✅
This comment was marked as resolved.
This comment was marked as resolved.
timvisee
commented
Mar 19, 2025
Comment on lines
+3
to
+5
| feature_flags: | ||
| use_new_shard_key_mapping_format: false | ||
|
|
Member
Author
There was a problem hiding this comment.
This idea immediately paid off, enabling the new shard key mapping logic turns out to be problematic with recent changes. A few consensus tests fail and it's not immediately clear to me what the problem is.
I'll disable it for now, and resolve the problems in a separate PR.
We can merge this one 👍
generall
approved these changes
Mar 19, 2025
9 tasks
6 tasks
9 tasks
timvisee
added a commit
that referenced
this pull request
Mar 21, 2025
* Add global feature flags structure, load on start * Feature flag use of new shard key format * Enable feature flag for new shard key format by default in dev builds * Prevent unwrapping everywhere; Single place for initialization (#6198) * Disable shard key feature flag by default for now, it causes problems --------- Co-authored-by: Jojii <[email protected]>
6 tasks
6 tasks
6 tasks
This file contains hidden or 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
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.
First iteration of a structure for configuring feature flags.
This allows us to explicitly set some flags during testing to enable new and upcoming features. It helps us test the features before releasing them, without having to change source code. I'd like to unify the interface for this. It's intentionally kept very simple.
For now this includes just one flag. I'd like to add a flag for using the new immutable ID tracker once merged.
In debug builds, all flags are enabled by default (as per
config/development.yaml). In chaos testing we'll probably enable all features by setting the appropriate environment variables.I chose to use this global structure because it turns out to be extremely cumbersome to propagate the flags to all places we might use it. I want to prevent having to propagate it (adding a lot of changes), having to undo all changes one or two versions later when we remove the flag. While I normally hate a global/static structure, I think this is a fair exception.
Let's use this PR as a place to discuss this feature.
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
cargo +nightly fmt --allcommand prior to submission?cargo clippy --all --all-featurescommand?