fix handling of empty string as checkpoint directory

Summary:
- made `CreateCheckpoint` properly return `InvalidArgument` when called with an empty directory. Previously it triggered an assertion failure due to a bug in the logic.
- made `ldb` set empty `checkpoint_dir` if that's what the user specifies, so that we can use it to properly test `CreateCheckpoint` in the future.

Differential Revision: D6874562

fbshipit-source-id: dcc1bd41768261d9338987fa7711444289707ed7
main
Andrew Kryczka 7 years ago committed by Facebook Github Bot
parent 5263da6396
commit 1960e73e21
  1. 1
      HISTORY.md
  2. 5
      tools/ldb_cmd.cc
  3. 7
      utilities/checkpoint/checkpoint_impl.cc
  4. 9
      utilities/checkpoint/checkpoint_test.cc

@ -4,6 +4,7 @@
* Iterator::SeekForPrev is now a pure virtual method. This is to prevent user who implement the Iterator interface fail to implement SeekForPrev by mistake.
* Add `include_end` option to make the range end exclusive when `include_end == false` in `DeleteFilesInRange()`.
* Add `CompactRangeOptions::allow_write_stall`, which makes `CompactRange` start working immediately, even if it causes user writes to stall. The default value is false, meaning we add delay to `CompactRange` calls until stalling can be avoided when possible. Note this delay is not present in previous RocksDB versions.
* Creating checkpoint with empty directory now returns `Status::InvalidArgument`; previously, it returned `Status::IOError`.
### New Features
* Improve the performance of iterators doing long range scans by using readahead.

@ -2616,10 +2616,7 @@ CheckPointCommand::CheckPointCommand(
: LDBCommand(options, flags, false /* is_read_only */,
BuildCmdLineOptions({ARG_CHECKPOINT_DIR})) {
auto itr = options.find(ARG_CHECKPOINT_DIR);
if (itr == options.end()) {
exec_state_ = LDBCommandExecuteResult::Failed(
"--" + ARG_CHECKPOINT_DIR + ": missing checkpoint directory");
} else {
if (itr != options.end()) {
checkpoint_dir_ = itr->second;
}
}

@ -62,9 +62,10 @@ Status CheckpointImpl::CreateCheckpoint(const std::string& checkpoint_dir,
size_t final_nonslash_idx = checkpoint_dir.find_last_not_of('/');
if (final_nonslash_idx == std::string::npos) {
// npos means it's only slashes, which means it's the root directory, but it
// shouldn't be because we verified above the directory doesn't exist.
assert(false);
// npos means it's only slashes or empty. Non-empty means it's the root
// directory, but it shouldn't be because we verified above the directory
// doesn't exist.
assert(checkpoint_dir.empty());
return Status::InvalidArgument("invalid checkpoint directory name");
}

@ -564,6 +564,15 @@ TEST_F(CheckpointTest, CurrentFileModifiedWhileCheckpointing2PC) {
delete txdb;
}
TEST_F(CheckpointTest, CheckpointInvalidDirectoryName) {
for (std::string checkpoint_dir : {"", "/", "////"}) {
Checkpoint* checkpoint;
ASSERT_OK(Checkpoint::Create(db_, &checkpoint));
ASSERT_TRUE(checkpoint->CreateCheckpoint("").IsInvalidArgument());
delete checkpoint;
}
}
} // namespace rocksdb
int main(int argc, char** argv) {

Loading…
Cancel
Save