search

basho / leveldb

Clone of http://code.google.com/p/leveldb/
BSD 3-Clause "New" or "Revised" License
408 stars 182 forks source link

Address case where non-tiered storage hot backup will not delete backup.4 directory. #223

Closed matthewvon closed 7 years ago

matthewvon commented 7 years ago

The hot backup code was extensively tested with the more difficult configuration of tiered storage. Therefore a bug in the simpler non-tiered storage was missed. The routine MakeTieredDbname() in db/filename.cc was originally designed to populate fast and slow tier names of the Options structure given a user's database path.

Hot backup is an internal user. It does not know how tiered storage was or was not constructed from components. Therefore, hot backup calls DestroyDB() (db/db_impl.cc) without a database name but with a full populated Options. This works just fine for tiered storage. It failed with non-tiered because MakeTieredDbname() returned a zero length string ... the string was therefore useless for deleting the backup.4 directory.

The PR adds a third case to MakeTieredDbname() and creates a unit test for the case within db/filename_test.cc

erikleitch commented 7 years ago

This is a simple fix to add a check for empty dbname in MakeTieredDbname() when options.tiered_fast_prefix.length() > 0.

The change itself seems fine to me, provided the underlying assumption is correct: ie, that hot backup always calls MakeTieredDbname with an empty string and non-empty fast_prefix, and that otherwise MakeTieredDbname gets called with a non-empty string and empty fast_prefix. This appears to be the case in hot_backup_test.cc, but without knowing more about tiered storage and hot backups, I can't verify the validity of that assumption in general. But I assume @matthewvon knows that that is the case.

I've certainly verified that the tests work as advertised (and that they don't work if you remove the check)

Totally minor note: I'd avoid mixing std::string::size() and std::string::length() in the same piece of code. They're synonymous, but may as well pick one and stick with it.

matthewvon commented 7 years ago

std::string::size() now used exclusively within MakeTieredDbname(). That is a Basho created routine. Did not review size() versus length() in remainder of Google created routines.

leveldb_ee tagging rebuilt. It was messed up when this branch was rebuilt.

hazen commented 7 years ago

Unit tests now build and pass locally

hazen commented 7 years ago

:+1: