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

@@ -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:
@@ -708,9 +716,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

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
 

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<std::string>* result);
+  Status GetChildren(const std::string& dir,
+                     std::vector<std::string>* 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);
+  Status GetAbsolutePath(const std::string& db_path,
-
+                         std::string* output_path) override;
-  // Generate unique id
-  std::string GenerateUniqueId();
 
   // Get default EnvLibrados
   static EnvLibrados* Default();

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) {
+  void Hint(AccessPattern /*pattern*/) { /* Do nothing */
-    /* 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(
+  Status PositionedAppend(const Slice& /* data */,
-    const Slice& /* data */,
+                          uint64_t /* offset */) override {
-    uint64_t /* offset */) {
     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<std::mutex> 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 {
+  bool IsSyncThreadSafe() const override { return true; }
-    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 {
+  bool use_direct_io() const override { return false; }
-    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<std::mutex> 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:
+ protected:
-  using WritableFile::Allocate;
   /**
    * @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):
+ public:
-    _io_ctx(io_ctx), _fid(fid) {}
+  explicit LibradosDirectory(librados::IoCtx* /*io_ctx*/, std::string fid)
+      : _fid(fid) {}
 
   // Fsync directory. Can be called concurrently from multiple threads.
-  Status Fsync() {
+  Status Fsync() { return Status::OK(); }
-    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(
+Status EnvLibrados::NewSequentialFile(const std::string& fname,
-  const std::string& fname,
+                                      std::unique_ptr<SequentialFile>* result,
-  std::unique_ptr<SequentialFile>* result,
+                                      const EnvOptions& /*options*/) {
-  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,
+    const std::string& fname, std::unique_ptr<RandomAccessFile>* result,
-  std::unique_ptr<RandomAccessFile>* result,
+    const EnvOptions& /*options*/) {
-  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(
+Status EnvLibrados::GetAbsolutePath(const std::string& db_path,
-  const std::string& db_path,
+                                    std::string* /*output_path*/) {
-  std::string* output_path)
+  (void)db_path;
-{
   LOG_DEBUG("[IO]%s\n", db_path.c_str());
   return Status::NotSupported();
 }

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 <stdio.h>
 
 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;
 }