[SERVER-33044] Move the fcvLock from being held for the duration of setFCV to only while setting targetVersion Created: 31/Jan/18 Updated: 16/Feb/18 Resolved: 16/Feb/18 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Dianna Hohensee (Inactive) | Assignee: | Dianna Hohensee (Inactive) |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Sprint: | Storage 2018-02-26 | ||||||||
| Participants: | |||||||||
| Description |
|
Currently setFCV and addShard take the fcvLock in exclusive mode so that they don't run together at all. This ticket must make sure that addShard is safe to run entirely before or after setFCV sets a targetVersion. addShard should be explored to ensure it's safe, and make necessary modifications. One such mod might be to make sure addShard sends a valid FCV when it calls setFCV on the new shard – if in transition, does the function used to acquire a version to put in the call return some non "3.6"/"4.0" string? shardCollection also takes the fcvLock, specifically for the v3.4 <=> v3.6 transition, due to UUIDs. This will be pulled out eventually ( |
| Comments |
| Comment by Dianna Hohensee (Inactive) [ 16/Feb/18 ] |
|
Closing as Won't Fix because we no longer want to minimize the duration of fcvLock in setFCV. It would require introducing a mutex replacement to prevent more than one thread executing the command at a time, and we don't want a mutex and fcvLock being held at the same time. |
| Comment by Dianna Hohensee (Inactive) [ 13/Feb/18 ] |
|
Note: we must reintroduce the original setFCV command specific mutex, '_kFeatureCompatibilityVersionLock', to prevent setFCV from running with itself. '_kFeatureCompatibilityVersionLock' was repurposed into the current 'fcvLock' here |
| Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ] |
|
I think it'd be better defensive programming to lock around any changes to the fcv state. In general, you should always take a lock when writing a variable that can be accessed from multiple threads (in this case, the variable is ServerGlobalParams::FeatureCompatibility::_version), regardless of whether the any code cares about the particular write being done. |
| Comment by Dianna Hohensee (Inactive) [ 01/Feb/18 ] |
|
We are not adding the lock around unsetting targetVersion. The existing use cases where this is needed are only differentiating between upgrading/upgraded and downgrading/downgraded. There is no need for locking unsetting because there is no need to lock the change from upgrading to upgraded, or the downgrading equivalent. |
| Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ] |
|
Also, I think shardCollection already correctly handles seeing an "upgrading" or "downgrading" state (it propagates the UUID if 'upgrading' or 'fullyUpgraded'). Perhaps addShard should do something similar - send setFCV(<theUpgradeVersion>) if upgrading or fullyUpgraded, else send setFCV(<theDowngradeVersion>)? |
| Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ] |
|
This ticket means "while setting or unsetting targetVersion", right? E.g., during setFCV: 1) take the fcvLock in X mode This way, any readers of the in-memory fcv state variable must see one of the fcv states: fully downgraded, upgrading, fully upgraded, or downgrading. |