[SERVER-77838] Consider changing or removing shouldBeFCVGated parameter Created: 06/Jun/23 Updated: 22/Jan/24 |
|
| Status: | Open |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Huayu Ouyang | Assignee: | Backlog - Replication Team |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | repl-shortlist | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Assigned Teams: |
Replication
|
||||||||||||
| Participants: | |||||||||||||
| Description |
|
The `shouldBeFCVGated` parameter was added as part of However, developers may not be aware that marking `shouldBeFCVGated: false` can be unsafe, and this flag does not necessarily convey this lack of safety the same way that isEnabledAndIgnoreFCVUnsafe does. We should consider whether this parameter should be changed or removed to be safer. |
| Comments |
| Comment by Max Hirschhorn [ 07/Jun/23 ] |
|
I agree with schwerin@mongodb.com about not wanting a function named "Unsafe" to be part of standard server development for certain types of new functionality/features. I think doing so confounds what patterns for how feature flags are used and checked is truly unsafe (e.g. corrupts data, returns incorrect results, etc.). I also do not want shouldBeFCVGated: false feature flags to be calling FeatureFlag::isEnabled(FCV) and looking like the return value might be dependent on the supplied FCV value but isn't. My current thinking is the server should have two different C++ types for feature flags and IDL can generate code which stamps out an instance of one type or the other type. (I'm not attached to their exact names.)
The IDL option for choosing between these two classes would express some necessary conditions to explain why it is safe to ignore the feature compatibility version. One idea for a name would be affectsDataReplication: false. My goal would be to get the project lead and code reviewers to be thinking "has my feature changes anything about the contents of the oplog?" And then to have that justification be written down. Separately, it looks like FeatureFlag::isEnabledUseDefaultFCVWhenUninitialized() is declared but not defined in feature_flag.cpp. Hopefully this means we can delete it? I'd also be curious to understand why the aggregation subsystem is using FeatureFlag::isEnabledOnVersion(FCV) when the documentation comment suggests it is only needing to be used within the context of the set_feature_compatibility_version_command.cpp due to how the in-memory state is modified. |
| Comment by Andy Schwerin [ 06/Jun/23 ] |
|
I don't know that "if your project doesn't require FCV gating, you check its feature flag with a function that has 'unsafe' in the name" does much more than dilute the power of the word "unsafe". I don't love the current state of affairs, but keeping a function named "Unsafe" that you're supposed to use for what I hope is actually most flag-gated features seems problem-prone. |