Skip to content

Make it harder to mistake deletions for legitimate writes in commit hooks#1221

Merged
achamayou merged 7 commits intomicrosoft:masterfrom
eddyashton:distinct_deletes2
May 28, 2020
Merged

Make it harder to mistake deletions for legitimate writes in commit hooks#1221
achamayou merged 7 commits intomicrosoft:masterfrom
eddyashton:distinct_deletes2

Conversation

@eddyashton
Copy link
Member

Fixes #1208.

The value type in a Write is now std::optional<V> instead of VersionV<V>. The version was supposed to be NoVersion to indicate a deletion, but all of our own hooks were ignoring version and accessing value directly. assuming it was a real written value. This is a breaking API change - even if you want to ignore the possibility of deletions, you now need to access the actual values by .value() (preferably gated by a check to .has_value(),) not .value.

Our hooks don't expect deletions and don't have anything sensible to do if they do occur, so they all simply throw an exception. It would be nice to have this expectation statically enforced (some kind of NoRemovals modifier type for a kv::Map, which returned TxViews with no remove method, and whose Write type could store values directly), but this is an unnecessary extra bit of machinery for now.

@eddyashton eddyashton requested a review from a team as a code owner May 27, 2020 15:20
@eddyashton eddyashton changed the title Make it harder to mistake deletions from writes in commit hooks Make it harder to mistake deletions for legitimate writes in commit hooks May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

distinct_deletes2@8746 aka 20200527.21 vs master ewma over 50 builds from 8546 to 8743
images

@eddyashton eddyashton force-pushed the distinct_deletes2 branch from 63156a9 to e305b76 Compare May 27, 2020 16:45
@achamayou achamayou merged commit 45f4585 into microsoft:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The write-set passed to KV commit-hooks doesn't clearly distinguish deletions

3 participants