setDDMode should set moveKeysLockWriteKey #1866
Conversation
After takeMoveKeysLock notes down the owner and the moveKeysLockWriteKey value, it monitors the above two in pollMoveKeysLock and checks if anything is changed, but setDDMode was not setting the moveKeysLockWriteKey and so a sequence like disable, enable and disable would not really disable DD.
|
This PR and the one from Jingyu(#1849) both indicate that |
There is already an encapsulation like |
| wrMyOwner << dataDistributionModeLock; | ||
| tr.set( moveKeysLockOwnerKey, wrMyOwner.toValue() ); | ||
| } | ||
| BinaryWriter wrMyOwner(Unversioned()); |
There was a problem hiding this comment.
I understand the moveKeysLockWriteKey, but why remove the !mode check?
There was a problem hiding this comment.
It seems the check is an optimization to avoid writing the ownerKeys while enabling DD mode. I felt an optimization without big value given the rarity of how many times we do this operation - so i thought simplification has more value than optimization here.
I think in the future I would add an assert that says that the ownerKey is still same as dataDistributionModeLock and then avoid writing the ownerKey again. (because between disable and enable, my understanding is that no one can change the ownership).
I am fine with putting the !mode check back too for now.
There was a problem hiding this comment.
that makes sense, I verified that the data distributor will re-take the lock after it has been re-enabled so I agree it should not change anything
4 participants
setDDMode disable enable disable - would not disable DD sometimes. setDDMode does not set the moveKeysLockWriteKey which can cause the above.
After takeMoveKeysLock notes down the moveKeysLockOwnerKey and the moveKeysLockWriteKey value, it monitors the above two in pollMoveKeysLock and checks if anything is changed, but setDDMode was not setting the moveKeysLockWriteKey and so a sequence like disable, enable and disable would not really disable DD unless the intermediate enable had a change in the moveKeysLockOwnerKey.
This will fix the issue #1865 .