From 281ac9c89e43640fad3f243108afc0389862fb57 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 15 Jun 2021 03:42:52 -0700 Subject: [PATCH] Add CreateFrom methods to Env/FileSystem (#8174) Summary: - Added CreateFromString method to Env and FilesSystem to replace LoadEnv/Load. This method/signature is a precursor to making these classes extend Customizable. - Added CreateFromSystem to Env. This method standardizes creating an Env from the environment variables. Previously, some places would check TEST_ENV_URI and others would also check TEST_FS_URI. Now the code is more command/standardized. - Added CreateFromFlags to Env. These method allows Env to be create from string options (such as GFLAGS options) in a more standard way. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8174 Reviewed By: zhichao-cao Differential Revision: D28999603 Pulled By: mrambacher fbshipit-source-id: 88e6911e7e91f908458a7fe10a20e93ecbc275fb --- db/column_family_test.cc | 12 +----- db/corruption_test.cc | 11 ++--- db/db_test_util.cc | 14 ++----- db/version_set_test.cc | 10 ++--- db_stress_tool/db_stress_tool.cc | 28 ++++--------- env/env.cc | 48 ++++++++++++++++++++- env/env_basic_test.cc | 25 +++++++---- env/file_system.cc | 12 +++++- include/rocksdb/env.h | 33 +++++++++++++++ include/rocksdb/file_system.h | 16 +++++++ options/db_options.cc | 5 ++- table/sst_file_reader_test.cc | 14 +++---- test_util/testutil.cc | 16 +++++++ test_util/testutil.h | 4 ++ tools/db_bench_tool.cc | 35 ++++++---------- tools/ldb_cmd.cc | 72 +++++++++----------------------- tools/ldb_cmd_test.cc | 7 +--- tools/sst_dump_test.cc | 8 ++-- tools/sst_dump_tool.cc | 45 +++++--------------- 19 files changed, 218 insertions(+), 197 deletions(-) diff --git a/db/column_family_test.cc b/db/column_family_test.cc index 09d055184..2db49813a 100644 --- a/db/column_family_test.cc +++ b/db/column_family_test.cc @@ -56,16 +56,8 @@ class ColumnFamilyTestBase : public testing::Test { public: explicit ColumnFamilyTestBase(uint32_t format) : rnd_(139), format_(format) { Env* base_env = Env::Default(); -#ifndef ROCKSDB_LITE - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (test_env_uri) { - Env* test_env = nullptr; - Status s = Env::LoadEnv(test_env_uri, &test_env, &env_guard_); - base_env = test_env; - EXPECT_OK(s); - EXPECT_NE(Env::Default(), base_env); - } -#endif // !ROCKSDB_LITE + EXPECT_OK( + test::CreateEnvFromSystem(ConfigOptions(), &base_env, &env_guard_)); EXPECT_NE(nullptr, base_env); env_ = new EnvCounter(base_env); env_->skip_fsync_ = true; diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 5c74905e7..cabf7e700 100644 --- a/db/corruption_test.cc +++ b/db/corruption_test.cc @@ -55,14 +55,9 @@ class CorruptionTest : public testing::Test { // bug in recovery code. Keep it 4 for now to make the test passes. tiny_cache_ = NewLRUCache(100, 4); Env* base_env = Env::Default(); -#ifndef ROCKSDB_LITE - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (test_env_uri) { - Status s = Env::LoadEnv(test_env_uri, &base_env, &env_guard_); - EXPECT_OK(s); - EXPECT_NE(Env::Default(), base_env); - } -#endif //! ROCKSDB_LITE + EXPECT_OK( + test::CreateEnvFromSystem(ConfigOptions(), &base_env, &env_guard_)); + EXPECT_NE(base_env, nullptr); env_ = new test::ErrorEnv(base_env); options_.wal_recovery_mode = WALRecoveryMode::kTolerateCorruptedTailRecords; options_.env = env_; diff --git a/db/db_test_util.cc b/db/db_test_util.cc index 0fbbd680a..ef10ec479 100644 --- a/db/db_test_util.cc +++ b/db/db_test_util.cc @@ -59,16 +59,8 @@ SpecialEnv::SpecialEnv(Env* base, bool time_elapse_only_sleep) DBTestBase::DBTestBase(const std::string path, bool env_do_fsync) : mem_env_(nullptr), encrypted_env_(nullptr), option_config_(kDefault) { Env* base_env = Env::Default(); -#ifndef ROCKSDB_LITE - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (test_env_uri) { - Env* test_env = nullptr; - Status s = Env::LoadEnv(test_env_uri, &test_env, &env_guard_); - base_env = test_env; - EXPECT_OK(s); - EXPECT_NE(Env::Default(), base_env); - } -#endif // !ROCKSDB_LITE + ConfigOptions config_options; + EXPECT_OK(test::CreateEnvFromSystem(config_options, &base_env, &env_guard_)); EXPECT_NE(nullptr, base_env); if (getenv("MEM_ENV")) { mem_env_ = new MockEnv(base_env); @@ -77,7 +69,7 @@ DBTestBase::DBTestBase(const std::string path, bool env_do_fsync) if (getenv("ENCRYPTED_ENV")) { std::shared_ptr provider; Status s = EncryptionProvider::CreateFromString( - ConfigOptions(), std::string("test://") + getenv("ENCRYPTED_ENV"), + config_options, std::string("test://") + getenv("ENCRYPTED_ENV"), &provider); encrypted_env_ = NewEncryptedEnv(mem_env_ ? mem_env_ : base_env, provider); } diff --git a/db/version_set_test.cc b/db/version_set_test.cc index 878f665c2..bb29b0f30 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -11,6 +11,7 @@ #include "db/db_impl/db_impl.h" #include "db/log_writer.h" +#include "rocksdb/convenience.h" #include "rocksdb/file_system.h" #include "table/block_based/block_based_table_factory.h" #include "table/mock_table.h" @@ -704,15 +705,10 @@ class VersionSetTestBase { write_buffer_manager_(db_options_.db_write_buffer_size), shutting_down_(false), mock_table_factory_(std::make_shared()) { - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (test_env_uri) { - Status s = Env::LoadEnv(test_env_uri, &env_, &env_guard_); - EXPECT_OK(s); - } else if (getenv("MEM_ENV")) { + EXPECT_OK(test::CreateEnvFromSystem(ConfigOptions(), &env_, &env_guard_)); + if (env_ == Env::Default() && getenv("MEM_ENV")) { env_guard_.reset(NewMemEnv(Env::Default())); env_ = env_guard_.get(); - } else { - env_ = Env::Default(); } EXPECT_NE(nullptr, env_); diff --git a/db_stress_tool/db_stress_tool.cc b/db_stress_tool/db_stress_tool.cc index 92597e309..cb489eb13 100644 --- a/db_stress_tool/db_stress_tool.cc +++ b/db_stress_tool/db_stress_tool.cc @@ -23,6 +23,7 @@ #ifdef GFLAGS #include "db_stress_tool/db_stress_common.h" #include "db_stress_tool/db_stress_driver.h" +#include "rocksdb/convenience.h" #ifndef NDEBUG #include "utilities/fault_injection_fs.h" #endif @@ -34,11 +35,6 @@ static std::shared_ptr env_wrapper_guard; static std::shared_ptr fault_env_guard; } // namespace -static Env* GetCompositeEnv(std::shared_ptr fs) { - static std::shared_ptr composite_env = NewCompositeEnv(fs); - return composite_env.get(); -} - KeyGenContext key_gen_ctx; int db_stress_tool(int argc, char** argv) { @@ -78,22 +74,14 @@ int db_stress_tool(int argc, char** argv) { if (!FLAGS_hdfs.empty()) { raw_env = new ROCKSDB_NAMESPACE::HdfsEnv(FLAGS_hdfs); - } else if (!FLAGS_env_uri.empty()) { - Status s = Env::LoadEnv(FLAGS_env_uri, &raw_env, &env_guard); - if (raw_env == nullptr) { - fprintf(stderr, "No Env registered for URI: %s\n", FLAGS_env_uri.c_str()); - exit(1); - } - } else if (!FLAGS_fs_uri.empty()) { - std::shared_ptr fs; - Status s = FileSystem::Load(FLAGS_fs_uri, &fs); - if (!s.ok()) { - fprintf(stderr, "Error: %s\n", s.ToString().c_str()); - exit(1); - } - raw_env = GetCompositeEnv(fs); } else { - raw_env = Env::Default(); + Status s = Env::CreateFromUri(ConfigOptions(), FLAGS_env_uri, FLAGS_fs_uri, + &raw_env, &env_guard); + if (!s.ok()) { + fprintf(stderr, "Error Creating Env URI: %s: %s\n", FLAGS_env_uri.c_str(), + s.ToString().c_str()); + exit(1); + } } #ifndef NDEBUG diff --git a/env/env.cc b/env/env.cc index 809b80568..77349fa68 100644 --- a/env/env.cc +++ b/env/env.cc @@ -16,6 +16,7 @@ #include "memory/arena.h" #include "options/db_options.h" #include "port/port.h" +#include "rocksdb/convenience.h" #include "rocksdb/options.h" #include "rocksdb/system_clock.h" #include "rocksdb/utilities/object_registry.h" @@ -582,11 +583,18 @@ Status Env::NewLogger(const std::string& fname, } Status Env::LoadEnv(const std::string& value, Env** result) { + return CreateFromString(ConfigOptions(), value, result); +} + +Status Env::CreateFromString(const ConfigOptions& config_options, + const std::string& value, Env** result) { Env* env = *result; Status s; #ifndef ROCKSDB_LITE + (void)config_options; s = ObjectRegistry::NewInstance()->NewStaticObject(value, &env); #else + (void)config_options; s = Status::NotSupported("Cannot load environment in LITE mode", value); #endif if (s.ok()) { @@ -597,18 +605,29 @@ Status Env::LoadEnv(const std::string& value, Env** result) { Status Env::LoadEnv(const std::string& value, Env** result, std::shared_ptr* guard) { + return CreateFromString(ConfigOptions(), value, result, guard); +} + +Status Env::CreateFromString(const ConfigOptions& config_options, + const std::string& value, Env** result, + std::shared_ptr* guard) { assert(result); + if (value.empty()) { + *result = Env::Default(); + return Status::OK(); + } Status s; #ifndef ROCKSDB_LITE Env* env = nullptr; std::unique_ptr uniq_guard; std::string err_msg; assert(guard != nullptr); + (void)config_options; env = ObjectRegistry::NewInstance()->NewObject(value, &uniq_guard, &err_msg); if (!env) { - s = Status::NotFound(std::string("Cannot load ") + Env::Type() + ": " + - value); + s = Status::NotSupported(std::string("Cannot load ") + Env::Type() + ": " + + value); env = Env::Default(); } if (s.ok() && uniq_guard) { @@ -618,6 +637,7 @@ Status Env::LoadEnv(const std::string& value, Env** result, *result = env; } #else + (void)config_options; (void)result; (void)guard; s = Status::NotSupported("Cannot load environment in LITE mode", value); @@ -625,6 +645,30 @@ Status Env::LoadEnv(const std::string& value, Env** result, return s; } +Status Env::CreateFromUri(const ConfigOptions& config_options, + const std::string& env_uri, const std::string& fs_uri, + Env** result, std::shared_ptr* guard) { + *result = config_options.env; + if (env_uri.empty() && fs_uri.empty()) { + // Neither specified. Use the default + guard->reset(); + return Status::OK(); + } else if (!env_uri.empty() && !fs_uri.empty()) { + // Both specified. Cannot choose. Return Invalid + return Status::InvalidArgument("cannot specify both fs_uri and env_uri"); + } else if (fs_uri.empty()) { // Only have an ENV URI. Create an Env from it + return CreateFromString(config_options, env_uri, result, guard); + } else { + std::shared_ptr fs; + Status s = FileSystem::CreateFromString(config_options, fs_uri, &fs); + if (s.ok()) { + guard->reset(new CompositeEnvWrapper(*result, fs)); + *result = guard->get(); + } + return s; + } +} + std::string Env::PriorityToString(Env::Priority priority) { switch (priority) { case Env::Priority::BOTTOM: diff --git a/env/env_basic_test.cc b/env/env_basic_test.cc index b11afb1fe..e8e3df5f6 100644 --- a/env/env_basic_test.cc +++ b/env/env_basic_test.cc @@ -75,19 +75,30 @@ namespace { // The purpose of returning an empty vector (instead of nullptr) is that gtest // ValuesIn() will skip running tests when given an empty collection. std::vector GetCustomEnvs() { - static Env* custom_env; static bool init = false; + static std::vector res; if (!init) { init = true; const char* uri = getenv("TEST_ENV_URI"); if (uri != nullptr) { - Env::LoadEnv(uri, &custom_env); + static std::shared_ptr env_guard; + static Env* custom_env; + Status s = + Env::CreateFromUri(ConfigOptions(), uri, "", &custom_env, &env_guard); + if (s.ok()) { + res.emplace_back(custom_env); + } + } + uri = getenv("TEST_FS_URI"); + if (uri != nullptr) { + static std::shared_ptr fs_env_guard; + static Env* fs_env; + Status s = + Env::CreateFromUri(ConfigOptions(), "", uri, &fs_env, &fs_env_guard); + if (s.ok()) { + res.emplace_back(fs_env); + } } - } - - std::vector res; - if (custom_env != nullptr) { - res.emplace_back(custom_env); } return res; } diff --git a/env/file_system.cc b/env/file_system.cc index 3e0c4d102..a6a2f3388 100644 --- a/env/file_system.cc +++ b/env/file_system.cc @@ -3,9 +3,11 @@ // COPYING file in the root directory) and Apache 2.0 License // (found in the LICENSE.Apache file in the root directory). // -#include "env/composite_env_wrapper.h" #include "rocksdb/file_system.h" + +#include "env/composite_env_wrapper.h" #include "options/db_options.h" +#include "rocksdb/convenience.h" #include "rocksdb/utilities/object_registry.h" namespace ROCKSDB_NAMESPACE { @@ -16,10 +18,18 @@ FileSystem::~FileSystem() {} Status FileSystem::Load(const std::string& value, std::shared_ptr* result) { + return CreateFromString(ConfigOptions(), value, result); +} + +Status FileSystem::CreateFromString(const ConfigOptions& config_options, + const std::string& value, + std::shared_ptr* result) { Status s; #ifndef ROCKSDB_LITE + (void)config_options; s = ObjectRegistry::NewInstance()->NewSharedObject(value, result); #else + (void)config_options; (void)result; s = Status::NotSupported("Cannot load FileSystem in LITE mode", value); #endif diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index 919a41ed7..a4463060a 100644 --- a/include/rocksdb/env.h +++ b/include/rocksdb/env.h @@ -64,6 +64,7 @@ class ThreadStatusUpdater; struct ThreadStatus; class FileSystem; class SystemClock; +struct ConfigOptions; const size_t kDefaultPageSize = 4 * 1024; @@ -169,12 +170,44 @@ class Env { static const char* Type() { return "Environment"; } // Loads the environment specified by the input value into the result + // The CreateFromString alternative should be used; this method may be + // deprecated in a future release. static Status LoadEnv(const std::string& value, Env** result); // Loads the environment specified by the input value into the result + // The CreateFromString alternative should be used; this method may be + // deprecated in a future release. static Status LoadEnv(const std::string& value, Env** result, std::shared_ptr* guard); + // Loads the environment specified by the input value into the result + // @see Customizable for a more detailed description of the parameters and + // return codes + // + // @param config_options Controls how the environment is loaded. + // @param value the name and associated properties for the environment. + // @param result On success, the environment that was loaded. + // @param guard If specified and the loaded environment is not static, + // this value will contain the loaded environment (guard.get() == + // result). + // @return OK If the environment was successfully loaded (and optionally + // prepared) + // @return not-OK if the load failed. + static Status CreateFromString(const ConfigOptions& config_options, + const std::string& value, Env** result); + static Status CreateFromString(const ConfigOptions& config_options, + const std::string& value, Env** result, + std::shared_ptr* guard); + + // Loads the environment specified by the env and fs uri. + // If both are specified, an error is returned. + // Otherwise, the environment is created by loading (via CreateFromString) + // the appropriate env/fs from the corresponding values. + static Status CreateFromUri(const ConfigOptions& options, + const std::string& env_uri, + const std::string& fs_uri, Env** result, + std::shared_ptr* guard); + // Return a default environment suitable for the current operating // system. Sophisticated users may wish to provide their own Env // implementation instead of relying on this default environment. diff --git a/include/rocksdb/file_system.h b/include/rocksdb/file_system.h index c45dc5d9e..025908e4f 100644 --- a/include/rocksdb/file_system.h +++ b/include/rocksdb/file_system.h @@ -46,6 +46,7 @@ class Slice; struct ImmutableDBOptions; struct MutableDBOptions; class RateLimiter; +struct ConfigOptions; using AccessPattern = RandomAccessFile::AccessPattern; using FileAttributes = Env::FileAttributes; @@ -209,9 +210,24 @@ class FileSystem { static const char* Type() { return "FileSystem"; } // Loads the FileSystem specified by the input value into the result + // The CreateFromString alternative should be used; this method may be + // deprecated in a future release. static Status Load(const std::string& value, std::shared_ptr* result); + // Loads the FileSystem specified by the input value into the result + // @see Customizable for a more detailed description of the parameters and + // return codes + // @param config_options Controls how the FileSystem is loaded + // @param value The name and optional properties describing the file system + // to load. + // @param result On success, returns the loaded FileSystem + // @return OK if the FileSystem was successfully loaded. + // @return not-OK if the load failed. + static Status CreateFromString(const ConfigOptions& options, + const std::string& value, + std::shared_ptr* result); + // Return a default fie_system suitable for the current operating // system. Sophisticated users may wish to provide their own Env // implementation instead of relying on this default file_system diff --git a/options/db_options.cc b/options/db_options.cc index 28d17e151..1e6113d37 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -418,11 +418,12 @@ static std::unordered_map OptionVerificationType::kNormal, (OptionTypeFlags::kDontSerialize | OptionTypeFlags::kCompareNever), // Parse the input value as an Env - [](const ConfigOptions& /*opts*/, const std::string& /*name*/, + [](const ConfigOptions& opts, const std::string& /*name*/, const std::string& value, void* addr) { auto old_env = static_cast(addr); // Get the old value Env* new_env = *old_env; // Set new to old - Status s = Env::LoadEnv(value, &new_env); // Update new value + Status s = Env::CreateFromString(opts, value, + &new_env); // Update new value if (s.ok()) { // It worked *old_env = new_env; // Update the old one } diff --git a/table/sst_file_reader_test.cc b/table/sst_file_reader_test.cc index bcd1af294..52cab2ab3 100644 --- a/table/sst_file_reader_test.cc +++ b/table/sst_file_reader_test.cc @@ -5,11 +5,13 @@ #ifndef ROCKSDB_LITE +#include "rocksdb/sst_file_reader.h" + #include #include "port/stack_trace.h" +#include "rocksdb/convenience.h" #include "rocksdb/db.h" -#include "rocksdb/sst_file_reader.h" #include "rocksdb/sst_file_writer.h" #include "table/sst_file_writer_collectors.h" #include "test_util/testharness.h" @@ -37,14 +39,8 @@ class SstFileReaderTest : public testing::Test { sst_name_ = test::PerThreadDBPath("sst_file"); Env* base_env = Env::Default(); - const char* test_env_uri = getenv("TEST_ENV_URI"); - if(test_env_uri) { - Env* test_env = nullptr; - Status s = Env::LoadEnv(test_env_uri, &test_env, &env_guard_); - base_env = test_env; - EXPECT_OK(s); - EXPECT_NE(Env::Default(), base_env); - } + EXPECT_OK( + test::CreateEnvFromSystem(ConfigOptions(), &base_env, &env_guard_)); EXPECT_NE(nullptr, base_env); env_ = base_env; options_.env = env_; diff --git a/test_util/testutil.cc b/test_util/testutil.cc index 34586bd24..450598cec 100644 --- a/test_util/testutil.cc +++ b/test_util/testutil.cc @@ -585,5 +585,21 @@ void DeleteDir(Env* env, const std::string& dirname) { TryDeleteDir(env, dirname).PermitUncheckedError(); } +Status CreateEnvFromSystem(const ConfigOptions& config_options, Env** result, + std::shared_ptr* guard) { + const char* env_uri = getenv("TEST_ENV_URI"); + const char* fs_uri = getenv("TEST_FS_URI"); + if (env_uri || fs_uri) { + return Env::CreateFromUri(config_options, + (env_uri != nullptr) ? env_uri : "", + (fs_uri != nullptr) ? fs_uri : "", result, guard); + } else { + // Neither specified. Use the default + *result = config_options.env; + guard->reset(); + return Status::OK(); + } +} + } // namespace test } // namespace ROCKSDB_NAMESPACE diff --git a/test_util/testutil.h b/test_util/testutil.h index 39fd5d9fb..c1dc09b0c 100644 --- a/test_util/testutil.h +++ b/test_util/testutil.h @@ -865,5 +865,9 @@ Status TryDeleteDir(Env* env, const std::string& dirname); // Delete a directory if it exists void DeleteDir(Env* env, const std::string& dirname); +// Creates an Env from the system environment by looking at the system +// environment variables. +Status CreateEnvFromSystem(const ConfigOptions& options, Env** result, + std::shared_ptr* guard); } // namespace test } // namespace ROCKSDB_NAMESPACE diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index 9e3306786..7424c1d53 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -45,6 +45,7 @@ #include "port/port.h" #include "port/stack_trace.h" #include "rocksdb/cache.h" +#include "rocksdb/convenience.h" #include "rocksdb/db.h" #include "rocksdb/env.h" #include "rocksdb/filter_policy.h" @@ -1068,15 +1069,6 @@ DEFINE_int32(thread_status_per_interval, 0, DEFINE_int32(perf_level, ROCKSDB_NAMESPACE::PerfLevel::kDisable, "Level of perf collection"); -#ifndef ROCKSDB_LITE -static ROCKSDB_NAMESPACE::Env* GetCompositeEnv( - std::shared_ptr fs) { - static std::shared_ptr composite_env = - ROCKSDB_NAMESPACE::NewCompositeEnv(fs); - return composite_env.get(); -} -#endif - static bool ValidateRateLimit(const char* flagname, double value) { const double EPSILON = 1e-10; if ( value < -EPSILON ) { @@ -7693,23 +7685,20 @@ int db_bench_tool(int argc, char** argv) { exit(1); } - if (!FLAGS_env_uri.empty()) { - Status s = Env::LoadEnv(FLAGS_env_uri, &FLAGS_env, &env_guard); - if (FLAGS_env == nullptr) { - fprintf(stderr, "No Env registered for URI: %s\n", FLAGS_env_uri.c_str()); + if (env_opts == 1) { + Status s = Env::CreateFromUri(ConfigOptions(), FLAGS_env_uri, FLAGS_fs_uri, + &FLAGS_env, &env_guard); + if (!s.ok()) { + fprintf(stderr, "Failed creating env: %s\n", s.ToString().c_str()); exit(1); } - } else if (!FLAGS_fs_uri.empty()) { - std::shared_ptr fs; - Status s = FileSystem::Load(FLAGS_fs_uri, &fs); - if (fs == nullptr) { - fprintf(stderr, "Error: %s\n", s.ToString().c_str()); - exit(1); - } - FLAGS_env = GetCompositeEnv(fs); } else if (FLAGS_simulate_hybrid_fs_file != "") { - FLAGS_env = GetCompositeEnv(std::make_shared( - FileSystem::Default(), FLAGS_simulate_hybrid_fs_file)); + //**TODO: Make the simulate fs something that can be loaded + // from the ObjectRegistry... + static std::shared_ptr composite_env = + NewCompositeEnv(std::make_shared( + FileSystem::Default(), FLAGS_simulate_hybrid_fs_file)); + FLAGS_env = composite_env.get(); } #endif // ROCKSDB_LITE if (FLAGS_use_existing_keys && !FLAGS_use_existing_db) { diff --git a/tools/ldb_cmd.cc b/tools/ldb_cmd.cc index 49d60debc..118f39802 100644 --- a/tools/ldb_cmd.cc +++ b/tools/ldb_cmd.cc @@ -287,16 +287,6 @@ LDBCommand* LDBCommand::SelectCommand(const ParsedParams& parsed_params) { return nullptr; } -static Env* GetCompositeEnv(std::shared_ptr fs) { - static std::shared_ptr composite_env = NewCompositeEnv(fs); - return composite_env.get(); -} - -static Env* GetCompositeBackupEnv(std::shared_ptr fs) { - static std::shared_ptr composite_backup_env = NewCompositeEnv(fs); - return composite_backup_env.get(); -} - /* Run the command, and return the execute result. */ void LDBCommand::Run() { if (!exec_state_.IsNotStarted()) { @@ -305,33 +295,13 @@ void LDBCommand::Run() { if (!options_.env || options_.env == Env::Default()) { Env* env = Env::Default(); - - if (!env_uri_.empty() && !fs_uri_.empty()) { - std::string err = - "Error: you may not specity both " - "fs_uri and fs_env."; - fprintf(stderr, "%s\n", err.c_str()); - exec_state_ = LDBCommandExecuteResult::Failed(err); + Status s = Env::CreateFromUri(config_options_, env_uri_, fs_uri_, &env, + &env_guard_); + if (!s.ok()) { + fprintf(stderr, "%s\n", s.ToString().c_str()); + exec_state_ = LDBCommandExecuteResult::Failed(s.ToString()); return; } - if (!env_uri_.empty()) { - Status s = Env::LoadEnv(env_uri_, &env, &env_guard_); - if (!s.ok() && !s.IsNotFound()) { - fprintf(stderr, "LoadEnv: %s\n", s.ToString().c_str()); - exec_state_ = LDBCommandExecuteResult::Failed(s.ToString()); - return; - } - } else if (!fs_uri_.empty()) { - std::shared_ptr fs; - Status s = FileSystem::Load(fs_uri_, &fs); - if (fs == nullptr) { - fprintf(stderr, "error: %s\n", s.ToString().c_str()); - exec_state_ = LDBCommandExecuteResult::Failed(s.ToString()); - return; - } - env = GetCompositeEnv(fs); - } - options_.env = env; } @@ -3218,19 +3188,17 @@ void BackupCommand::DoCommand() { } fprintf(stdout, "open db OK\n"); - Env* custom_env = nullptr; - if (!backup_fs_uri_.empty()) { - std::shared_ptr fs; - Status s = FileSystem::Load(backup_fs_uri_, &fs); - if (fs == nullptr) { + Env* custom_env = backup_env_guard_.get(); + if (custom_env == nullptr) { + Status s = + Env::CreateFromUri(config_options_, backup_env_uri_, backup_fs_uri_, + &custom_env, &backup_env_guard_); + if (!s.ok()) { exec_state_ = LDBCommandExecuteResult::Failed(s.ToString()); return; } - custom_env = GetCompositeBackupEnv(fs); - } else { - Env::LoadEnv(backup_env_uri_, &custom_env, &backup_env_guard_); - assert(custom_env != nullptr); } + assert(custom_env != nullptr); BackupableDBOptions backup_options = BackupableDBOptions(backup_dir_, custom_env); @@ -3265,19 +3233,17 @@ void RestoreCommand::Help(std::string& ret) { } void RestoreCommand::DoCommand() { - Env* custom_env = nullptr; - if (!backup_fs_uri_.empty()) { - std::shared_ptr fs; - Status s = FileSystem::Load(backup_fs_uri_, &fs); - if (fs == nullptr) { + Env* custom_env = backup_env_guard_.get(); + if (custom_env == nullptr) { + Status s = + Env::CreateFromUri(config_options_, backup_env_uri_, backup_fs_uri_, + &custom_env, &backup_env_guard_); + if (!s.ok()) { exec_state_ = LDBCommandExecuteResult::Failed(s.ToString()); return; } - custom_env = GetCompositeBackupEnv(fs); - } else { - Env::LoadEnv(backup_env_uri_, &custom_env, &backup_env_guard_); - assert(custom_env != nullptr); } + assert(custom_env != nullptr); std::unique_ptr restore_engine; Status status; diff --git a/tools/ldb_cmd_test.cc b/tools/ldb_cmd_test.cc index f601502d6..ed70ca39c 100644 --- a/tools/ldb_cmd_test.cc +++ b/tools/ldb_cmd_test.cc @@ -12,6 +12,7 @@ #include "env/composite_env_wrapper.h" #include "file/filename.h" #include "port/stack_trace.h" +#include "rocksdb/convenience.h" #include "rocksdb/file_checksum.h" #include "rocksdb/utilities/options_util.h" #include "test_util/sync_point.h" @@ -31,12 +32,8 @@ class LdbCmdTest : public testing::Test { LdbCmdTest() : testing::Test() {} Env* TryLoadCustomOrDefaultEnv() { - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (!test_env_uri) { - return Env::Default(); - } Env* env = Env::Default(); - Env::LoadEnv(test_env_uri, &env, &env_guard_); + EXPECT_OK(test::CreateEnvFromSystem(ConfigOptions(), &env, &env_guard_)); return env; } diff --git a/tools/sst_dump_test.cc b/tools/sst_dump_test.cc index 5cc215362..3cbfcd3cc 100644 --- a/tools/sst_dump_test.cc +++ b/tools/sst_dump_test.cc @@ -10,11 +10,12 @@ #ifndef ROCKSDB_LITE #include -#include "rocksdb/sst_dump_tool.h" #include "file/random_access_file_reader.h" #include "port/stack_trace.h" +#include "rocksdb/convenience.h" #include "rocksdb/filter_policy.h" +#include "rocksdb/sst_dump_tool.h" #include "table/block_based/block_based_table_factory.h" #include "table/table_builder.h" #include "test_util/testharness.h" @@ -56,10 +57,7 @@ class SSTDumpToolTest : public testing::Test { public: SSTDumpToolTest() : env_(Env::Default()) { - const char* test_env_uri = getenv("TEST_ENV_URI"); - if (test_env_uri) { - Env::LoadEnv(test_env_uri, &env_, &env_guard_); - } + EXPECT_OK(test::CreateEnvFromSystem(ConfigOptions(), &env_, &env_guard_)); test_dir_ = test::PerThreadDBPath(env_, "sst_dump_test_db"); Status s = env_->CreateDirIfMissing(test_dir_); EXPECT_OK(s); diff --git a/tools/sst_dump_tool.cc b/tools/sst_dump_tool.cc index 9e808c88f..195747e0a 100644 --- a/tools/sst_dump_tool.cc +++ b/tools/sst_dump_tool.cc @@ -132,16 +132,8 @@ bool ParseIntArg(const char* arg, const std::string arg_name, } } // namespace -static ROCKSDB_NAMESPACE::Env* GetCompositeEnv( - std::shared_ptr fs) { - static std::shared_ptr composite_env = - ROCKSDB_NAMESPACE::NewCompositeEnv(fs); - return composite_env.get(); -} - int SSTDumpTool::Run(int argc, char const* const* argv, Options options) { - const char* env_uri = nullptr; - const char* fs_uri = nullptr; + std::string env_uri, fs_uri; const char* dir_or_file = nullptr; uint64_t read_num = std::numeric_limits::max(); std::string command; @@ -354,32 +346,17 @@ int SSTDumpTool::Run(int argc, char const* const* argv, Options options) { // If caller of SSTDumpTool::Run(...) does not specify a different env other // than Env::Default(), then try to load custom env based on env_uri/fs_uri. // Otherwise, the caller is responsible for creating custom env. - - if (env_uri && fs_uri) { - fprintf(stderr, "cannot specify --fs_uri and --env_uri.\n\n"); - exit(1); - } - - if (!options.env || options.env == ROCKSDB_NAMESPACE::Env::Default()) { - Env* env = Env::Default(); - if (env_uri) { - Status s = Env::LoadEnv(env_uri ? env_uri : "", &env, &env_guard); - if (!s.ok() && !s.IsNotFound()) { - fprintf(stderr, "LoadEnv: %s\n", s.ToString().c_str()); - exit(1); - } - } else if (fs_uri) { - std::shared_ptr fs; - Status s = FileSystem::Load(fs_uri, &fs); - if (fs == nullptr) { - fprintf(stderr, "FileSystem Load: %s\n", s.ToString().c_str()); - exit(1); - } - env = GetCompositeEnv(fs); + { + ConfigOptions config_options; + config_options.env = options.env; + Status s = Env::CreateFromUri(config_options, env_uri, fs_uri, &options.env, + &env_guard); + if (!s.ok()) { + fprintf(stderr, "CreateEnvFromUri: %s\n", s.ToString().c_str()); + exit(1); + } else { + fprintf(stdout, "options.env is %p\n", options.env); } - options.env = env; - } else { - fprintf(stdout, "options.env is %p\n", options.env); } std::vector filenames;