From f55aab3d184df7d8c03522c17527bd8a7953cd16 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 29 Oct 2021 08:17:24 -0700 Subject: [PATCH] Fix EnvLibrados and add to CI (#9088) Summary: This feature was not part of any common or CI build, so no surprise it broke. Now we can at least ensure compilation. I don't know how to run the test successfully (missing config file) so it is bypassed for now. Fixes https://github.com/facebook/rocksdb/issues/9078 Pull Request resolved: https://github.com/facebook/rocksdb/pull/9088 Test Plan: CI Reviewed By: mrambacher Differential Revision: D32009467 Pulled By: pdillinger fbshipit-source-id: 3e0d1e5fde7f0ece703d48a81479e1cc7392c25c --- .circleci/config.yml | 16 +++-- Makefile | 1 + include/rocksdb/utilities/env_librados.h | 13 ++-- utilities/env_librados.cc | 85 +++++++++++------------- utilities/env_librados_test.cc | 7 +- 5 files changed, 62 insertions(+), 60 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index b39532496..5a6392862 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -98,6 +98,13 @@ commands: command: | sudo apt-get update -y && sudo apt-get install -y libbenchmark-dev + install-librados: + steps: + - run: + name: Install librados + command: | + sudo apt-get update -y && sudo apt-get install -y librados-dev + upgrade-cmake: steps: - run: @@ -171,14 +178,15 @@ jobs: - run: make V=1 J=32 -j32 check | .circleci/cat_ignore_eagain - post-steps - build-linux-mem-env: + build-linux-mem-env-librados: machine: image: ubuntu-1604:202104-01 resource_class: 2xlarge steps: - pre-steps - install-gflags - - run: MEM_ENV=1 make V=1 J=32 -j32 check | .circleci/cat_ignore_eagain + - install-librados + - run: MEM_ENV=1 ROCKSDB_USE_LIBRADOS=1 make V=1 J=32 -j32 check | .circleci/cat_ignore_eagain - post-steps build-linux-encrypted-env: @@ -698,9 +706,9 @@ workflows: jobs: - build-linux-cmake - build-linux-cmake-ubuntu-20 - build-linux-mem-env: + build-linux-mem-env-librados: jobs: - - build-linux-mem-env + - build-linux-mem-env-librados build-linux-encrypted-env: jobs: - build-linux-encrypted-env diff --git a/Makefile b/Makefile index 274864378..dcd6cf903 100644 --- a/Makefile +++ b/Makefile @@ -222,6 +222,7 @@ am__v_AR_1 = ifdef ROCKSDB_USE_LIBRADOS LIB_SOURCES += utilities/env_librados.cc +TEST_MAIN_SOURCES += utilities/env_librados_test.cc LDFLAGS += -lrados endif diff --git a/include/rocksdb/utilities/env_librados.h b/include/rocksdb/utilities/env_librados.h index 361217c62..77fb82558 100644 --- a/include/rocksdb/utilities/env_librados.h +++ b/include/rocksdb/utilities/env_librados.h @@ -76,7 +76,8 @@ class EnvLibrados : public EnvWrapper { // Store in *result the names of the children of the specified directory. // The names are relative to "dir". // Original contents of *results are dropped. - Status GetChildren(const std::string& dir, std::vector* result); + Status GetChildren(const std::string& dir, + std::vector* result) override; // Delete the named file. Status DeleteFile(const std::string& fname) override; @@ -116,18 +117,16 @@ class EnvLibrados : public EnvWrapper { // to go away. // // May create the named file if it does not already exist. - Status LockFile(const std::string& fname, FileLock** lock); + Status LockFile(const std::string& fname, FileLock** lock) override; // Release the lock acquired by a previous successful call to LockFile. // REQUIRES: lock was returned by a successful LockFile() call // REQUIRES: lock has not already been unlocked. - Status UnlockFile(FileLock* lock); + Status UnlockFile(FileLock* lock) override; // Get full directory name for this db. - Status GetAbsolutePath(const std::string& db_path, std::string* output_path); - - // Generate unique id - std::string GenerateUniqueId(); + Status GetAbsolutePath(const std::string& db_path, + std::string* output_path) override; // Get default EnvLibrados static EnvLibrados* Default(); diff --git a/utilities/env_librados.cc b/utilities/env_librados.cc index 53d05f856..bca9b5586 100644 --- a/utilities/env_librados.cc +++ b/utilities/env_librados.cc @@ -172,7 +172,7 @@ public: * * @return [description] */ - Status InvalidateCache(size_t offset, size_t length) { + Status InvalidateCache(size_t /*offset*/, size_t /*length*/) { return Status::OK(); } }; @@ -237,8 +237,7 @@ public: }; //enum AccessPattern { NORMAL, RANDOM, SEQUENTIAL, WILLNEED, DONTNEED }; - void Hint(AccessPattern pattern) { - /* Do nothing */ + void Hint(AccessPattern /*pattern*/) { /* Do nothing */ } /** @@ -250,7 +249,7 @@ public: * * @return [description] */ - Status InvalidateCache(size_t offset, size_t length) { + Status InvalidateCache(size_t /*offset*/, size_t /*length*/) { return Status::OK(); } }; @@ -315,6 +314,7 @@ class LibradosWritableFile : public WritableFile { Sync(); } + using WritableFile::Append; /** * @brief append data to file * @details @@ -324,7 +324,7 @@ class LibradosWritableFile : public WritableFile { * @param data [description] * @return [description] */ - Status Append(const Slice& data) { + Status Append(const Slice& data) override { // append buffer LOG_DEBUG("[IN] %i | %s\n", (int)data.size(), data.data()); int r = 0; @@ -341,14 +341,14 @@ class LibradosWritableFile : public WritableFile { return err_to_status(r); } + using WritableFile::PositionedAppend; /** * @brief not supported * @details [long description] * @return [description] */ - Status PositionedAppend( - const Slice& /* data */, - uint64_t /* offset */) { + Status PositionedAppend(const Slice& /* data */, + uint64_t /* offset */) override { return Status::NotSupported(); } @@ -359,7 +359,7 @@ class LibradosWritableFile : public WritableFile { * @param size [description] * @return [description] */ - Status Truncate(uint64_t size) { + Status Truncate(uint64_t size) override { LOG_DEBUG("[IN]%lld|%lld|%lld\n", (long long)size, (long long)_file_size, (long long)_buffer_size); int r = 0; @@ -391,7 +391,7 @@ class LibradosWritableFile : public WritableFile { * @details [long description] * @return [description] */ - Status Close() { + Status Close() override { LOG_DEBUG("%s | %lld | %lld\n", _hint.c_str(), (long long)_buffer_size, (long long)_file_size); return Sync(); } @@ -402,7 +402,7 @@ class LibradosWritableFile : public WritableFile { * * @return [description] */ - Status Flush() { + Status Flush() override { librados::AioCompletion *write_completion = librados::Rados::aio_create_completion(); int r = 0; @@ -425,7 +425,7 @@ class LibradosWritableFile : public WritableFile { * @details initiate an aio write and wait for result * @return [description] */ - Status Sync() { // sync data + Status Sync() override { // sync data int r = 0; std::lock_guard lock(_mutex); @@ -441,18 +441,14 @@ class LibradosWritableFile : public WritableFile { * @details [long description] * @return true if Sync() and Fsync() are safe to call concurrently with Append()and Flush(). */ - bool IsSyncThreadSafe() const { - return true; - } + bool IsSyncThreadSafe() const override { return true; } /** * @brief Indicates the upper layers if the current WritableFile implementation uses direct IO. * @details [long description] * @return [description] */ - bool use_direct_io() const { - return false; - } + bool use_direct_io() const override { return false; } /** * @brief Get file size @@ -460,7 +456,7 @@ class LibradosWritableFile : public WritableFile { * This API will use cached file_size. * @return [description] */ - uint64_t GetFileSize() { + uint64_t GetFileSize() override { LOG_DEBUG("%lld|%lld\n", (long long)_buffer_size, (long long)_file_size); std::lock_guard lock(_mutex); @@ -478,7 +474,7 @@ class LibradosWritableFile : public WritableFile { * * @return [description] */ - size_t GetUniqueId(char* id, size_t max_size) const { + size_t GetUniqueId(char* id, size_t max_size) const override { // All fid has the same db_id prefix, so we need to ignore db_id prefix size_t s = std::min(max_size, _fid.size()); strncpy(id, _fid.c_str() + (_fid.size() - s), s); @@ -495,11 +491,10 @@ class LibradosWritableFile : public WritableFile { * * @return [description] */ - Status InvalidateCache(size_t offset, size_t length) { + Status InvalidateCache(size_t /*offset*/, size_t /*length*/) override { return Status::OK(); } - using WritableFile::RangeSync; /** * @brief No RangeSync support, just call Sync() * @details [long description] @@ -509,12 +504,11 @@ class LibradosWritableFile : public WritableFile { * * @return [description] */ - Status RangeSync(off_t offset, off_t nbytes) { + Status RangeSync(uint64_t /*offset*/, uint64_t /*nbytes*/) override { return Sync(); } -protected: - using WritableFile::Allocate; + protected: /** * @brief noop * @details [long description] @@ -524,7 +518,7 @@ protected: * * @return [description] */ - Status Allocate(off_t offset, off_t len) { + Status Allocate(uint64_t /*offset*/, uint64_t /*len*/) override { return Status::OK(); } }; @@ -533,16 +527,14 @@ protected: // Directory object represents collection of files and implements // filesystem operations that can be executed on directories. class LibradosDirectory : public Directory { - librados::IoCtx * _io_ctx; std::string _fid; -public: - explicit LibradosDirectory(librados::IoCtx * io_ctx, std::string fid): - _io_ctx(io_ctx), _fid(fid) {} + + public: + explicit LibradosDirectory(librados::IoCtx* /*io_ctx*/, std::string fid) + : _fid(fid) {} // Fsync directory. Can be called concurrently from multiple threads. - Status Fsync() { - return Status::OK(); - } + Status Fsync() { return Status::OK(); } }; // Identifies a locked file. @@ -552,8 +544,8 @@ class LibradosFileLock : public FileLock { const std::string _obj_name; const std::string _lock_name; const std::string _cookie; - int lock_state; -public: + + public: LibradosFileLock( librados::IoCtx * io_ctx, const std::string obj_name): @@ -870,11 +862,9 @@ librados::IoCtx* EnvLibrados::_GetIoctx(const std::string& fpath) { * @param options [description] * @return [description] */ -Status EnvLibrados::NewSequentialFile( - const std::string& fname, - std::unique_ptr* result, - const EnvOptions& options) -{ +Status EnvLibrados::NewSequentialFile(const std::string& fname, + std::unique_ptr* result, + const EnvOptions& /*options*/) { LOG_DEBUG("[IN]%s\n", fname.c_str()); std::string dir, file, fid; split(fname, &dir, &file); @@ -914,10 +904,8 @@ Status EnvLibrados::NewSequentialFile( * @return [description] */ Status EnvLibrados::NewRandomAccessFile( - const std::string& fname, - std::unique_ptr* result, - const EnvOptions& options) -{ + const std::string& fname, std::unique_ptr* result, + const EnvOptions& /*options*/) { LOG_DEBUG("[IN]%s\n", fname.c_str()); std::string dir, file, fid; split(fname, &dir, &file); @@ -1374,6 +1362,8 @@ Status EnvLibrados::LinkFile( const std::string& src, const std::string& target_in) { + (void)src; + (void)target_in; LOG_DEBUG("[IO]%s => %s\n", src.c_str(), target_in.c_str()); return Status::NotSupported(); } @@ -1455,10 +1445,9 @@ Status EnvLibrados::UnlockFile(FileLock* lock) * * @return [description] */ -Status EnvLibrados::GetAbsolutePath( - const std::string& db_path, - std::string* output_path) -{ +Status EnvLibrados::GetAbsolutePath(const std::string& db_path, + std::string* /*output_path*/) { + (void)db_path; LOG_DEBUG("[IO]%s\n", db_path.c_str()); return Status::NotSupported(); } diff --git a/utilities/env_librados_test.cc b/utilities/env_librados_test.cc index 3b86881ac..86e3297ee 100644 --- a/utilities/env_librados_test.cc +++ b/utilities/env_librados_test.cc @@ -1133,6 +1133,11 @@ TEST_F(EnvLibradosMutipoolTest, DBTransactionDB) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); + if (getenv("CIRCLECI")) { + fprintf(stderr, + "TODO: get env_librados_test working in CI. Skipping for now.\n"); + return 0; + } return RUN_ALL_TESTS(); } @@ -1140,7 +1145,7 @@ int main(int argc, char** argv) { #include int main(int argc, char** argv) { - fprintf(stderr, "SKIPPED as EnvMirror is not supported in ROCKSDB_LITE\n"); + fprintf(stderr, "SKIPPED as EnvLibrados is not supported in ROCKSDB_LITE\n"); return 0; }