Skip to content

Commit

Permalink
Simplify migration to FileSystem API (#6552)
Browse files Browse the repository at this point in the history
Summary:
The current Env/FileSystem API separation has a couple of issues -
1. It requires the user to specify 2 options - ```Options::env``` and ```Options::file_system``` - which means they have to make code changes to benefit from the new APIs. Furthermore, there is a risk of accessing the same APIs in two different ways, through Env in the old way and through FileSystem in the new way. The two may not always match, for example, if env is ```PosixEnv``` and FileSystem is a custom implementation. Any stray RocksDB calls to env will use the ```PosixEnv``` implementation rather than the file_system implementation.
2. There needs to be a simple way for the FileSystem developer to instantiate an Env for backward compatibility purposes.

This PR solves the above issues and simplifies the migration in the following ways -
1. Embed a shared_ptr to the ```FileSystem``` in the ```Env```, and remove ```Options::file_system``` as a configurable option. This way, no code changes will be required in application code to benefit from the new API. The default Env constructor uses a ```LegacyFileSystemWrapper``` as the embedded ```FileSystem```.
1a. - This also makes it more robust by ensuring that even if RocksDB
  has some stray calls to Env APIs rather than FileSystem, they will go
  through the same object and thus there is no risk of getting out of
  sync.
2. Provide a ```NewCompositeEnv()``` API that can be used to construct a
PosixEnv with a custom FileSystem implementation. This eliminates an
indirection to call Env APIs, and relieves the FileSystem developer of
the burden of having to implement wrappers for the Env APIs.
3. Add a couple of missing FileSystem APIs - ```SanitizeEnvOptions()``` and
```NewLogger()```

Tests:
1. New unit tests
2. make check and make asan_check
Pull Request resolved: facebook/rocksdb#6552

Reviewed By: riversand963

Differential Revision: D20592038

Pulled By: anand1976

fbshipit-source-id: c3801ad4153f96d21d5a3ae26c92ba454d1bf1f7
Signed-off-by: Changlong Chen <levisonchen@live.cn>
  • Loading branch information
anand76 authored and mm304321141 committed Jun 23, 2021
1 parent 2325d45 commit a2c85d3
Showing 1 changed file with 17 additions and 4 deletions.
21 changes: 17 additions & 4 deletions include/rocksdb/file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ struct FileOptions : EnvOptions {

FileOptions(const EnvOptions& opts)
: EnvOptions(opts) {}

FileOptions(const FileOptions& opts)
: EnvOptions(opts), io_options(opts.io_options) {}
};

// A structure to pass back some debugging information from the FileSystem
Expand Down Expand Up @@ -263,7 +266,7 @@ class FileSystem {
const std::string& old_fname,
const FileOptions& file_opts,
std::unique_ptr<FSWritableFile>* result,
IODebugContext* dbg) = 0;
IODebugContext* dbg);

// Open `fname` for random read and write, if file doesn't exist the file
// will be created. On success, stores a pointer to the new file in
Expand Down Expand Up @@ -465,6 +468,10 @@ class FileSystem {
std::string* output_path,
IODebugContext* dbg) = 0;

// Sanitize the FileOptions. Typically called by a FileOptions/EnvOptions
// copy constructor
virtual void SanitizeFileOptions(FileOptions* /*opts*/) const {}

// OptimizeForLogRead will create a new FileOptions object that is a copy of
// the FileOptions in the parameters, but is optimized for reading log files.
virtual FileOptions OptimizeForLogRead(const FileOptions& file_options) const;
Expand Down Expand Up @@ -1001,11 +1008,13 @@ class FSDirectory {
class FileSystemWrapper : public FileSystem {
public:
// Initialize an EnvWrapper that delegates all calls to *t
explicit FileSystemWrapper(FileSystem* t) : target_(t) {}
explicit FileSystemWrapper(std::shared_ptr<FileSystem> t) : target_(t) {}
~FileSystemWrapper() override {}

const char* Name() const override { return target_->Name(); }

// Return the target to which this Env forwards all calls
FileSystem* target() const { return target_; }
FileSystem* target() const { return target_.get(); }

// The following text is boilerplate that forwards all methods to target()
IOStatus NewSequentialFile(const std::string& f,
Expand Down Expand Up @@ -1149,6 +1158,10 @@ class FileSystemWrapper : public FileSystem {
return target_->NewLogger(fname, options, result, dbg);
}

void SanitizeFileOptions(FileOptions* opts) const override {
target_->SanitizeFileOptions(opts);
}

FileOptions OptimizeForLogRead(
const FileOptions& file_options) const override {
return target_->OptimizeForLogRead(file_options);
Expand Down Expand Up @@ -1182,7 +1195,7 @@ class FileSystemWrapper : public FileSystem {
}

private:
FileSystem* target_;
std::shared_ptr<FileSystem> target_;
};

class FSSequentialFileWrapper : public FSSequentialFile {
Expand Down

0 comments on commit a2c85d3

Please sign in to comment.