[SERVER-31968] Refactor FCV component code in order to remove circular dependencies around feature_compatibility_version.h/cpp Created: 14/Nov/17  Updated: 06/Dec/22  Resolved: 19/Aug/19

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

Type: Task Priority: Minor - P4
Reporter: Xiangyu Yao (Inactive) Assignee: Backlog - Storage Execution Team
Resolution: Won't Do Votes: 0
Labels: neweng, todo_in_code
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-33561 Add a featureCompatibilityVersion doc... Closed
depends on SERVER-33562 Move FeatureCompatibilityVersion::kCo... Closed
Related
related to SERVER-43447 Complete TODO listed in SERVER-31968 Closed
Assigned Teams:
Storage Execution
Sprint: Storage 2018-01-01, Storage 2018-01-15
Participants:

 Description   

Right now, we define FeatureCompatibilityVersion::kCollection in feature_compatibility_version.h. It is problematic because:
1. It's a StringData but it really should be a NamespaceString ("admin.system.version"). Also the name is ambiguous: its name implies the string value should be "system.version" but the db name is in it.
2. feature_compatibility_version is pretty high-level since it makes calls to high-level functions such as _runUpdateCommand. Some lower-level code (such as op_observer_impl.cpp) refer to this constant (kCollection) so it causes an unnecessary circular dependency issue.

Update
Further work required to successfully remove the circular dependencies and clean up the code. See below comment. Title has been updated.



 Comments   
Comment by Dianna Hohensee (Inactive) [ 28/Feb/18 ]

Making this a master ticket.

Created SERVER-33561 and SERVER-33562 to cover the work we want to be done as part of the 4.0 upgrade/downgrade project (PM-936). feature_compatibility_version.h/cpp is fairly high level file, so lower level code can't include it as a dependency. This work will move string constants and toString/parse helpers out of feature_compatibility_version.h/cpp into a light weight low dependency file, which can be included anywhere.

Comment by Dianna Hohensee (Inactive) [ 07/Feb/18 ]

So moving kCollection out of FeatureCompatibilityVersion doesn't suffice to remove the circular dependency issues. kParameter and kVersion are also used in various files in which kCollection is used .

The issue appears to lie in FeatureCompatibilityVersion being simultaneously a FCV doc parser (should be an FCVType) and an OpObserver. Then there's FeatureCompatibilityVersionCommandParser and ServerGlobalParams::FeatureCompatibilityVersion off in left field. Need to rearrange the code.

Initial, not completely verified or thought out, proposal

  • FeatureCompatibilityVersion::parse and FeatureCompatibilityVersionCommandParser::kVersion34, kVersion36, kVersionUpgradingTo36, etc., go into a new parse file. Maybe throw FeatureCompatibilityVersionCommandParser::extractVersionFromCommand functionality into the same file? FeatureCompatibilityVersionCommandParser is a light (low dependencies) file, so might be clean – except commands.h is included... FeatureCompatibilityVersion::toString should probably go in here, too.
  • FeatureCompatibilityVersion::setIfCleanStartup and isCleanStartUp should likely split off into its own file, because it has different dependencies than the OpObserver and parser functionality present in feature_compatibility_version.h/cpp? It has StorageInterface and StorageEngine uses.
  • fcvLock should be usable from wherever FCV is checked, which lives in server_options.h, so maybe move that there? Consider putting internalValidateFeaturesAsMaster in server_options.h, too, but I'm not sure what server_options.h logically encompasses, so there might be a better place for it – or not, and it stays where it is.
  • That leaves setTargetUpgrade/Downgrade and op oberserver code, which should all go together. Should consider moving setTargetUpgrade/Downgrade to the setFCV command, if it drags annoying dependencies somehow – haven't looked.
Generated at Thu Feb 08 04:28:45 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.