Skip to content

Commit

Permalink
Fix crash test with backup as read-only DB (#8161)
Browse files Browse the repository at this point in the history
Summary:
Forgot to re-test crash test after adding read-only filesystem
enforcement to facebook/rocksdb#8142. The problem is ReadOnlyFileSystem would reject
CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix
that is better for users is to allow CreateDirIfMissing in
ReadOnlyFileSystem if the directory exists, so that they don't cause a
failure on using create_if_missing with opening backups as read-only
DBs. Added this option test to the unit test (in addition to being in the
crash test).

Also fixed a couple of lints.

And some better messaging from 'make format' so that when you run it
with uncommitted changes, it's clear that it's only checking the
uncommitted changes.

Pull Request resolved: facebook/rocksdb#8161

Test Plan: local blackbox_crash_test with amplified backup_one_in

Reviewed By: ajkr

Differential Revision: D27614409

Pulled By: pdillinger

fbshipit-source-id: 63ccb626c7e34c200d61c6bca2a8f60da9015179
Signed-off-by: Changlong Chen <levisonchen@live.cn>
  • Loading branch information
pdillinger authored and Changlong Chen committed Jun 18, 2021
1 parent df3ca99 commit f7cbfa3
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
17 changes: 12 additions & 5 deletions env/fs_readonly.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class ReadOnlyFileSystem : public FileSystemWrapper {
}

public:
ReadOnlyFileSystem(const std::shared_ptr<FileSystem>& base)
explicit ReadOnlyFileSystem(const std::shared_ptr<FileSystem>& base)
: FileSystemWrapper(base) {}

IOStatus NewWritableFile(const std::string& /*fname*/,
Expand Down Expand Up @@ -61,10 +61,17 @@ class ReadOnlyFileSystem : public FileSystemWrapper {
IODebugContext* /*dbg*/) override {
return FailReadOnly();
}
IOStatus CreateDirIfMissing(const std::string& /*dirname*/,
const IOOptions& /*options*/,
IODebugContext* /*dbg*/) override {
return FailReadOnly();
IOStatus CreateDirIfMissing(const std::string& dirname,
const IOOptions& options,
IODebugContext* dbg) override {
// Allow if dir already exists
bool is_dir = false;
IOStatus s = IsDirectory(dirname, options, &is_dir, dbg);
if (s.ok() && is_dir) {
return s;
} else {
return FailReadOnly();
}
}
IOStatus DeleteDir(const std::string& /*dirname*/,
const IOOptions& /*options*/,
Expand Down
2 changes: 1 addition & 1 deletion env/fs_remap.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace ROCKSDB_NAMESPACE {
// guarantees.
class RemapFileSystem : public FileSystemWrapper {
public:
RemapFileSystem(const std::shared_ptr<FileSystem>& base);
explicit RemapFileSystem(const std::shared_ptr<FileSystem>& base);

protected:
// Returns status and mapped-to path in the wrapped filesystem.
Expand Down

0 comments on commit f7cbfa3

Please sign in to comment.