Skip to content

Add global feature flags#6194

Merged
timvisee merged 5 commits intodevfrom
global-feature-flags
Mar 19, 2025
Merged

Add global feature flags#6194
timvisee merged 5 commits intodevfrom
global-feature-flags

Conversation

@timvisee
Copy link
Copy Markdown
Member

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:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

@timvisee timvisee force-pushed the global-feature-flags branch from c46ab20 to 8c7ccf6 Compare March 18, 2025 15:59
@qdrant qdrant deleted a comment from coderabbitai Bot Mar 18, 2025
coderabbitai[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@coszio coszio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@JojiiOfficial JojiiOfficial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea!
I created some minor suggestions in #6198 but they're not necessary.

@coderabbitai

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Comment thread config/development.yaml
Comment on lines +3 to +5
feature_flags:
use_new_shard_key_mapping_format: false

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@timvisee timvisee merged commit 75eb0b3 into dev Mar 19, 2025
@timvisee timvisee deleted the global-feature-flags branch March 19, 2025 10:15
@coderabbitai coderabbitai Bot mentioned this pull request Mar 19, 2025
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants