basho / bitcask

because you need another a key/value storage engine
1.28k stars 171 forks source link

Bugfix/key transform crash #186

Closed engelsanchez closed 10 years ago

engelsanchez commented 10 years ago

When merging completely expired files, we fold over them issuing conditional keydir deletes on the entries of the file. That way, if any of them are still present in the keydir, they will be deleted now instead of the usual lazy delete on read, when we detect they are expired.

Previously, the code was not accounting for the new {tombstone, Key} return format when folding over the hintfile. A recent change in 1.7.0 added a tombstone bit flag to hintfiles, so now we can tell we are dealing with a tombstone without inspecting the data file. The tuple was being passed whole to the userdefined key transformation function, causing a crash. With this change, we simply skip over tombstones. It makes no sense to try to delete them from the keydir.

This fixes basho/bitcask#185 in the 1.7 branch (Bitcask shipped with Riak 2.0.0). When merged, this should be ported or merged to the develop branch also.

jonmeredith commented 10 years ago

+1 5fbb8f72c765c484124193bb553cbd3605d5f4c2

Reviewed code, accounts for the tombstone response returned by bitcask_fileops:fold_keys. Audited code for other instances of bitcask_fileops and those folders already accounted for {tombstone, …}. Ran the eunit test with coverage testing enabled and confirmed it hit the case and failed in the commit before the fix. Full eunit test passes for me. Will see if borshop agrees with me.

engelsanchez commented 10 years ago

@borshop merge