[SERVER-59820] Correct readability regression in FCV constants Created: 07/Sep/21  Updated: 13/Sep/21  Resolved: 13/Sep/21

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Daniel Gottlieb (Inactive) Assignee: Xuerui Fa
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Sprint: Repl 2021-09-20
Participants:

 Description   

We used to explicitly integrate the FCV upgrade/downgrade states inside the enum name.

The new system omits those details which makes communicating more difficult. The new raw values:

    kFullyDowngradedTo_5_0,
    kUpgradingFrom_5_0_To_5_1,
    kDowngradingFrom_5_1_To_5_0,
    kVersion_5_1,

The new fcv agnostic, synthetic values:

    static constexpr auto kLatest = FeatureCompatibilityVersion::kVersion_5_1;
    static constexpr auto kLastContinuous = FeatureCompatibilityVersion::kFullyDowngradedTo_5_0;
    static constexpr auto kLastLTS = FeatureCompatibilityVersion::kFullyDowngradedTo_5_0;

I propose changing:

  • kVersion_5_1 to kFullyUpgradedTo_5_1
  • kLatest to kFullyUpgradedToLatest
  • kLastContinuous to kFullyDowngradedToLastContinuous
  • kLastLTS to kFullyDowngradedToLastLTS


 Comments   
Comment by Daniel Gottlieb (Inactive) [ 13/Sep/21 ]

I'm closing this ticket after an offline discussion. I do believe there's a legitimate readability regression for existing developers that were intimately familiar with the old FCV API. But the new API is self-consistent and I see no reason why it'd be tricky for an uninitiated developer to understand the new world of FCV naming.

Comment by Xuerui Fa [ 07/Sep/21 ]

The transitionary FCVs exist for transitions between generic FCVs (kLastLTS, kLastContinuous, kLatest). For the 5.2 release, lastContinuous will be 5.1, and last LTS will still be 5.0. So I think we'd have both of those constants you listed above. I also agree that we shouldn't be labeling lastContinuous values with "kFullyDowngraded...", as users could be upgrading or downgrading to them.

For the FCV enum:
Currently, the oldest FCV value is already labeled as "kFullyDowngradedTo...". I'm fine with renaming the most recent value to "kFullyUpgradedTo...".

For generic FCVs:
The lastLTS FCV should always point to the oldest FCV accepted by the node, while the latest FCV should point to the most recent FCV for the node. As a result, I'm not too sure if there is much gained by renaming the generic FCVs, unless server engineers aren't aware of the mechanisms of these generic FCV constants. If there is this knowledge gap, maybe we can leave a comment explaining the values of the generic FCV constants?

I agree that it's a bit more annoying for server devs to figure out exactly which FCV enum the generic FCVs are pointing to, it's an unfortunate side effect of generating code automatically.

Comment by Daniel Gottlieb (Inactive) [ 07/Sep/21 ]

Does that mean the following are only in reference to lts/continuous steps?

    kUpgradingFrom_5_0_To_5_1,
    kDowngradingFrom_5_1_To_5_0,

As in, when 5.2 is released, those values will instead read:

    kUpgradingFrom_5_0_To_5_2,
    kDowngradingFrom_5_2_To_5_0,

Comment by Jason Chan [ 07/Sep/21 ]

After we branch and release, wouldn't we change kFullyUpgradedTo_5_1 to kFullyDowngradedTo_5_1? Continuous delivery still allows for downgrading "interim" versions and "interim" versions may still introduce on-disk changes?

When designing FCV for continuous delivery, we intentionally removed the fullyUpgraded/fullyDowngraded from the interim versions is so we didn't have to change all the kFullyUpgraded to kFullyDowngraded prefixes on each release. Since we aim to release more frequently, we looked to eliminate as many of the manual steps required for server FCV updates after continuous release as possible.

Comment by Daniel Gottlieb (Inactive) [ 07/Sep/21 ]

Would you mind expanding on what the value add is for using this terminology?

As per the following snippet, I couldn't tell what kLastLTS meant. Are we fully downgraded or perhaps in the process of downgrading?

It seems the adjectives have changed? I'm looking for "fully" or "target". I also can't seem to grep for GenericFCV and find where those constants are defined to see what the analogous words are.

The value add would be not having to compile the source code and navigating into the build directory to find the answer to something that used to take less time. Thus, it being a regression.

I don't think we should change kVersion_5_1 to kFullyUpgradedTo_5_1 since after we branch and release 5.1, it would be possible to downgrade from latest (5.2) to last-continuous (5.1).

After we branch and release, wouldn't we change kFullyUpgradedTo_5_1 to kFullyDowngradedTo_5_1? Continuous delivery still allows for downgrading "interim" versions and "interim" versions may still introduce on-disk changes?

Comment by Jason Chan [ 07/Sep/21 ]

I don't think we should change kVersion_5_1 to kFullyUpgradedTo_5_1 since after we branch and release 5.1, it would be possible to downgrade from latest (5.2) to last-continuous (5.1). On another note, we allow upgrading from lastLTS to lastContinuous when a config server is issuing setFCV as part of the addShard command, so 'fullyDowngradedToLastContinuous' may be confusing in that context.

I think it could make sense to add 'FullyUpgraded' and 'FullyDowngraded' to the LTS versions if we think it provides value.

Comment by Xuerui Fa [ 07/Sep/21 ]

daniel.gottlieb Thanks for filing this ticket! Would you mind expanding on what the value add is for using this terminology?

CC jason.chan

Generated at Thu Feb 08 05:48:12 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.