[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:
The new fcv agnostic, synthetic values:
I propose changing:
|
| 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: For generic FCVs: 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?
As in, when 5.2 is released, those values will instead read:
| ||||
| Comment by Jason Chan [ 07/Sep/21 ] | ||||
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 ] | ||||
As per the following snippet, I couldn't tell what kLastLTS meant. Are we fully downgraded or perhaps in the process of downgrading?
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.
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 |