Simplify migration to FileSystem API #6552

anand1976 · 2020-03-17T17:22:54Z

The current Env/FileSystem API separation has a couple of issues -

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.
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 -

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.
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.
Add a couple of missing FileSystem APIs - SanitizeEnvOptions() and
NewLogger()

Tests:

New unit tests
make check and make asan_check

riversand963

Thanks @anand1976. I left a few initial comments.

riversand963 · 2020-03-18T00:15:43Z

env/composite_env_wrapper.h

@@ -293,6 +293,10 @@ class CompositeEnvWrapper : public Env {
  // calls to env, and all file operations to fs
  explicit CompositeEnvWrapper(Env* env, FileSystem* fs)
      : env_target_(env), fs_env_target_(fs) {}
+  explicit CompositeEnvWrapper(Env* env, std::unique_ptr<FileSystem>&& fs)
+      : env_target_(env), fs_env_ptr_(std::move(fs)), fs_env_target_(fs_env_ptr_.get()) {}


Can CompositeEnvWrapper keep fs_env_ptr_ and remove fs_env_target_, since the latter is just fs_env_ptr_.get()?

Yes, it can. Will make the change.

riversand963 · 2020-03-18T00:19:06Z

env/composite_env_wrapper.h

@@ -293,6 +293,10 @@ class CompositeEnvWrapper : public Env {
  // calls to env, and all file operations to fs
  explicit CompositeEnvWrapper(Env* env, FileSystem* fs)
      : env_target_(env), fs_env_target_(fs) {}
+  explicit CompositeEnvWrapper(Env* env, std::unique_ptr<FileSystem>&& fs)


Should fs be passed to Env (base class constructor) as well?

riversand963 · 2020-03-18T00:29:50Z

include/rocksdb/env.h

  // If you're adding methods here, remember to add them to EnvWrapper too.

 protected:
  // The pointer to an internal structure that will update the
  // status of each thread.
  ThreadStatusUpdater* thread_status_updater_;
+
+  // Pointer to the underlying FileSystem implementation
+  std::shared_ptr<FileSystem> file_system_;


What is the difference between Env::file_system_ and CompositeEnv::fs_env_ptr?

CompositeEnv can use Env::file_system_. I believe there's no need for fs_env_ptr_.

mrambacher

It seems like you should have:

FileSystem: Abstract class/interface that defines the FS APIs
FileSystemWrapper: Concrete class that is-a/has-a FileSystem
Env: Abstract class that defines some APIs, has a FileSystem variable, and some other abstract methods
EnvWrapper: Concrete class that is-a/has-a FileSystem
- The Env should have a constructor that takes an Env and one that takes an Env and a FileSystem
LegacyFileSystem: Takes an Env and converts it to a FileSystem'
PosixFileSystem: Split this class out of the PosixEnv into the FileSystem elements
PosixEnv: Same as it was before without the FileSystem code. Passes the PosixFileSystem to the Env parent.

It seems like if you took a few more of the Env (MockEnv, SleepEnv) and tried to convert them into being an Env with a FileSystem, the structure and requirements of the new code would become clearer.

mrambacher · 2020-03-18T01:54:03Z

db/error_handler_fs_test.cc

@@ -150,12 +150,12 @@ class ErrorHandlerFSListener : public EventListener {
 };

 TEST_F(DBErrorHandlingFSTest, FLushWriteError) {
-  FaultInjectionTestFS* fault_fs =
-      new FaultInjectionTestFS(FileSystem::Default().get());
+  std::shared_ptr<FaultInjectionTestFS> fault_fs(new FaultInjectionTestFS(FileSystem::Default().get()));


Shouldn't the wrapped FileSystem be a std::shared_ptr and not a FileSystem * ? Otherwise we are back to issues with who controls its lifetime...

Yes, it can be a shared_ptr. Let me see if I can change it. If its not that easy, maybe a separate PR.

mrambacher · 2020-03-18T01:57:54Z

env/composite_env_wrapper.h

@@ -293,6 +293,10 @@ class CompositeEnvWrapper : public Env {
  // calls to env, and all file operations to fs
  explicit CompositeEnvWrapper(Env* env, FileSystem* fs)
      : env_target_(env), fs_env_target_(fs) {}
+  explicit CompositeEnvWrapper(Env* env, std::unique_ptr<FileSystem>&& fs)
+      : env_target_(env), fs_env_ptr_(std::move(fs)), fs_env_target_(fs_env_ptr_.get()) {}
+  explicit CompositeEnvWrapper(Env* env, std::shared_ptr<FileSystem> fs)


What is the difference between a CompositeEnvWrapper and an EnvWrapper? Why do we need both>?

EnvWrapper redirects to a base Env, CompositeEnvWrapper redirects to a base Env and a base FileSystem. EnvWrapper could be made to conditionally redirect file operations to an Env or a FileSystem depending on how its constructed, but I choose to keep the two separate.

Why does an EnvWrapper need to do anything with a FileSystem? I do not see the need for both classes and think it just adds an extra layer and more confusion. Or is the plan to deprecate the EnvWrapper code?

Since an Env has a FileSystem, there is need to wrap the FileSystem or related calls in the EnvWrapper class (just like the CompositeEnvWrapper does). You can create an EnvWrapper with just an Env (in which case it gets the FileSystem from the input arg) or with both an Env and a FileSystem. In both cases, it passes the FileSystem to the Env parent.

The plan is to deprecate the file related APIs in both Env and EnvWrapper eventually.

You have a good point about wrapping the FileSystem related calls in EnvWrapper. Taking MockEnv as an example, it could be split into two -

class MockFileSystem : public FileSystemWrapper { MockFileSystem(std::shared_ptr<FileSystem> base_fs) : FileSystemWrapper(base_fs) {}

and

class MockEnv : public EnvWrapper { MockEnv(Env* base_env, std::shared_ptr<FileSystem> base_fs) : Env(std::make_shared<MockFileSystem>(base_fs)) {}

and FileSystem related calls in EnvWrapper would look something like this -

class EnvWrapper : public Env { EnvWrapper(Env* base_env, std::shared<FileSystem> base_fs) : Env(base_fs), target_(base_env), constructed_with_fs_(true) {} Status NewSequentialFile(...) { if (constructed_with_fs_) { return file_system_->NewSequentialFile(...); } else { return target_->NewSequentialFile(...); } }

There may be other ways of doing it - need to think about it more. Will defer this to a separate PR. Right now, my focus is on porting an Env+FS implementation that inherits directly from Env, very similar to HdfsEnv.

Why isn't the model:

In the base env classes, the "legacy file system" calls are no longer pure virtual. Instead, they are wrapped as "file_system->DoSomething()"

There is a "LegacyEnvWrapper" that converts a legacy env into a file system.

The Env class has a constructor that takes a FileSystem as an argument

The no-arg Env constructor creates a LegacyEnvWrapper from "this"

The EnvWrapper ONLY wraps the non-legacy FileSystem calls. All other calls are handled by the base Env class, which will redirect it to the underlying FileSystem

The EnvWrapper was two constructors, one that takes an Env and one that takes an Env and a FileSystem.

There is no need for three classes (CompositeWrapper, EnvWrapper, and Env) as they all overlap.

And there should not need to be modifications to the base Posix class, other than to separate out the PosixFileSystem, but it sounds like that is already done.

I would also make the case that it is very hard to prove that what you done works until you have covered a few more of the cases -- like making a few of the existing implementations follow this model. Ideally, I would think you would do ALL of them to prevent code rot...

In the base env classes, the "legacy file system" calls are no longer pure virtual. Instead, they are wrapped as "file_system->DoSomething()"

The EnvWrapper ONLY wraps the non-legacy FileSystem calls. All other calls are handled by the base Env class, which will redirect it to the underlying FileSystem

I don't disagree. This may be what we end up with. However, I'm not comfortable with making the calls non-pure virtual yet until the new APIs are proven with a FileSystem implementation that actually uses some of the features of the new API (its WIP currently). MockEnv etc are not good candidates. I understand they need to be accommodated, but they're not useful in proving the new API.

CompositeEnvWrapper and PosixEnv are internal to RocksDB. If/when we go with the approach you suggested, it should be very cheap to refactor them.

mrambacher · 2020-03-18T02:03:25Z

include/rocksdb/env.h

@@ -530,12 +533,19 @@ class Env {

  virtual void SanitizeEnvOptions(EnvOptions* /*env_opts*/) const {}

+  // Get the FileSystem implementation this Env was constructed with. It
+  // could be a fully implemented one, or a wrapper class around the Env
+  std::shared_ptr<FileSystem> GetFileSystem();


shouldn't this be const std::shared_ptr& GetFileSystem() const?

Makes sense, since we can avoid some overhead if the caller just wants to dereference it immediately.

mrambacher · 2020-03-18T02:05:47Z

env/env_posix.cc

-    : CompositeEnvWrapper(this, FileSystem::Default().get()),
-      thread_pools_(Priority::TOTAL),
-      allow_non_owner_access_(true) {
+    : CompositeEnvWrapper(this, FileSystem::Default()),


Why doesn't PosixEnv just inherit from Env? What is gained by the Wrapper?

Backward compatibility. The old/to-be-deprecated Env APIs should still work for the time being.

Can you explain the compatibility? I do not see where that is coming from.

Env::New*File APIs should call the corresponding FileSystem::New*File APIs. Since the former are pure virtual functions in Env, CompositeEnvWrapper provides the necessary wrappers to redirect those calls to the underlying FileSystem. Eventually, WinEnv should also inherit from it.

mrambacher · 2020-03-18T02:06:16Z

env/env_posix.cc

@@ -428,6 +420,15 @@ PosixEnv::PosixEnv()
  thread_status_updater_ = CreateThreadStatusUpdater();
 }

+PosixEnv::PosixEnv(const PosixEnv* default_env, std::shared_ptr<FileSystem> fs)


I do not think this constructor is or should be necessary. Who calls it and when?

mrambacher · 2020-03-18T02:08:02Z

env/env_posix.cc

+std::unique_ptr<Env> NewCompositeEnv(std::shared_ptr<FileSystem> fs) {
+  PosixEnv* default_env = static_cast<PosixEnv*>(Env::Default());
+  return std::unique_ptr<Env>(new PosixEnv(default_env, fs));
 }


This seems like the wrong approach to me. Can you explain why we need this method at all?

See previous comment about backward compatibility. A FileSystem developer can call this to allocate a Env with PosixEnv for non-FS operations. What is wrong about this approach? Do you have an alternative?

This logic is already accomplished through the CompositeEnv class and doesn't need to be pushed into the PosixEnv one.

I think what you are after is that there should be an Env::Default(std::shared_ptr) API added. This way, you could create a "PosixEnv" using a non-Posix File system. Additionally, there should be a std::shared_ptr & FileSystem::Default() call that returns a PosixFileSystem.

The FileSystem::Default() API already exists. NewCompositeEnv(std::shared_ptr) is basically the Env::Default(std::shared_ptr) API that you're referring to.

mrambacher · 2020-03-18T02:09:08Z

db/version_set.cc

@@ -4771,9 +4771,10 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
  Status s;
  {
    std::unique_ptr<FSSequentialFile> file;
-    s = options.file_system->NewSequentialFile(
+    std::shared_ptr<FileSystem> fs = options.env->GetFileSystem();


auto & fs = ... ?

anand1976 · 2020-03-19T00:05:39Z

@mrambacher I took a look at MockEnv (can't find SleepEnv in the codebase). Agree that EnvWrapper should have a constructor that takes an Env pointer and a FileSystem.

Summary: Simplfy 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. 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()

riversand963

LGTM with a few minor comments. Thanks @anand1976 for the PR

db/error_handler_fs_test.cc

env/env_test.cc

env/env.cc

mrambacher

@mrambacher I took a look at MockEnv (can't find SleepEnv in the codebase). Agree that EnvWrapper should have a constructor that takes an Env pointer and a FileSystem.

Try MockTimeEnv. You should validate that your model works for both Envs that should be FileSystems and those that are really overriding Env methods

facebook-github-bot

@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-03-23T22:44:46Z

@anand1976 has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-03-23T22:44:49Z

@anand1976 has updated the pull request. Re-import the pull request

facebook-github-bot

@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-03-24T07:29:13Z

@anand1976 merged this pull request in a9d168c.

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>

anand1976 requested review from siying and riversand963 March 17, 2020 17:22

facebook-github-bot added the CLA Signed label Mar 17, 2020

riversand963 reviewed Mar 18, 2020

View reviewed changes

mrambacher reviewed Mar 18, 2020

View reviewed changes

riversand963 self-requested a review March 19, 2020 16:24

anand76 added 2 commits March 19, 2020 11:34

Address review comments

4679599

anand1976 force-pushed the env_fs_creation branch from 0e37640 to 4679599 Compare March 19, 2020 18:46

riversand963 approved these changes Mar 20, 2020

View reviewed changes

db/error_handler_fs_test.cc Outdated Show resolved Hide resolved

env/env_test.cc Show resolved Hide resolved

env/env.cc Show resolved Hide resolved

mrambacher reviewed Mar 20, 2020

View reviewed changes

Formatting changes and implement default ReuseWritableFile()

e793b54

facebook-github-bot reviewed Mar 23, 2020

View reviewed changes

Add a test and one missing method in FileSystemWrapper

aa45edb

facebook-github-bot reviewed Mar 23, 2020

View reviewed changes

facebook-github-bot closed this in a9d168c Mar 24, 2020

facebook-github-bot added the Merged label Mar 24, 2020

mrambacher mentioned this pull request Apr 9, 2020

Skew between DBOptions and ImmutableDBOptions #6677

Open

rockeet mentioned this pull request Apr 16, 2020

Proposal: Separate Env into Thread and FileSystem #4746

Closed

ajkr mentioned this pull request Aug 23, 2021

Rocks DB crash when used via JNI. Version: 6.20.3 #8617

Closed

Simplify migration to FileSystem API #6552

Simplify migration to FileSystem API #6552

Conversation

anand1976 commented Mar 17, 2020

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anand1976 Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anand1976 commented Mar 19, 2020

riversand963 left a comment

Choose a reason for hiding this comment

mrambacher left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 23, 2020

facebook-github-bot commented Mar 23, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 24, 2020

anand1976 Mar 18, 2020 •

edited

Loading