Support custom env in sst_dump #5845
Conversation
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
3 times, most recently
from
7b636cd
to
314508f
Compare
| s = ObjectRegistry::NewInstance()->NewSharedObject<Env>(value, guard); | ||
| #else | ||
| (void) guard; | ||
| s = Status::NotSupported("Cannot load environment in LITE mode: ", value); |
There was a problem hiding this comment.
Would that make sst_dump unusable in LITE build?
Should we fallback to default Env in LITE build?
There was a problem hiding this comment.
In LITE move, the user should only use LoadEnv(const std::string& value, Env** result). I should put #ifndef...#else outside this entire overloaded version.
There was a problem hiding this comment.
I'm not sure I'm following. Env::LoadEnv() is called in LDBCommand::OpenDB(), so LITE build will always call it and by default it will fail. Did I miss anything?
There was a problem hiding this comment.
Or, maybe combined version
Status Env::LoadEnv(const std::string&, Env** result, std::shared_ptr<Env>*)
The caller should know what to do with the returned pointers.
There was a problem hiding this comment.
The entire LDBCommand is in #ifndef ROCKSDB_LITE section.
There was a problem hiding this comment.
I still find it a little weird to have two versions of LoadEnv. However, I don't think we can simply remove the original one. I am going to modify the newly-added one so that it will look like the following.
Status LoadEnv(const std::string, Env** result, std::shared_ptr<Env>* guard);
Then we can remove the original one during 7.0 release.
Does this sound good?
There was a problem hiding this comment.
Status LoadEnv(const std::string, Env** result, std::shared_ptr<Env>* guard) is definitely a better API than what we we have here.
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
912c2fa
to
4429cb3
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
4429cb3
to
1e15664
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
1e15664
to
f151e30
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
f151e30
to
db80dd7
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
3 similar comments
|
@riversand963 has updated the pull request. Re-import the pull request |
|
@riversand963 has updated the pull request. Re-import the pull request |
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
72b948d
to
0f3f906
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
Summary: This PR allows for the creation of custom env when using sst_dump. If the user does not set options.env or set options.env to nullptr, then sst_dump will automatically try to create a custom env depending on the path to the sst file or db directory. In order to use this feature, the user must call ObjectRegistry::Register() beforehand. Test Plan (on devserver): ``` $make all && make check ``` All tests must pass to ensure this change does not break anything.
riversand963
force-pushed
the
sst_dump_support_custom_env
branch
from
0f3f906
to
1608497
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 merged this pull request in 167cdc9. |
Summary: This PR allows for the creation of custom env when using sst_dump. If the user does not set options.env or set options.env to nullptr, then sst_dump will automatically try to create a custom env depending on the path to the sst file or db directory. In order to use this feature, the user must call ObjectRegistry::Register() beforehand. Test Plan (on devserver): ``` $make all && make check ``` All tests must pass to ensure this change does not break anything. Pull Request resolved: facebook#5845 Differential Revision: D17678038 Pulled By: riversand963 fbshipit-source-id: 58ecb4b3f75246d52b07c4c924a63ee61c1ee626
Summary: This PR allows for the creation of custom env when using sst_dump. If
the user does not set options.env or set options.env to nullptr, then sst_dump
will automatically try to create a custom env depending on the path to the sst
file or db directory. In order to use this feature, the user must call
ObjectRegistry::Register() beforehand.
Test Plan (on devserver):
All tests must pass to ensure this change does not break anything.