1. 09 Jun, 2020 1 commit
    • Zhichao Cao's avatar
      decouple the dependency of trace_analyzer_test unit test (#6941) · fb08330f
      Zhichao Cao authored
      Since gflags use the global variable to store the flags passed in. In the unit test, if we git one flag per unit test, the result is that all the flags are combined together in the following tests. Therefore, it has the dependency. In this PR, we pass the full arguments each time to ensure that the old arguments will be overwritten by the new one such that the dependency is removed.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6941
      Test Plan: make asan_check. run each unit test in trace_analyzer_test independently and in arbitrary orders.
      Reviewed By: pdillinger
      Differential Revision: D21909176
      Pulled By: zhichao-cao
      fbshipit-source-id: dca550a0a4a205c30faa620e258a020a3b5b4e13
  2. 08 Jun, 2020 1 commit
    • Yanqin Jin's avatar
      Remove unnecessary inclusion of version_edit.h in env (#6952) · 3020df9d
      Yanqin Jin authored
      In db_options.c, we should avoid including header files in the `db` directory to avoid introducing unnecessary dependency. The reason why `version_edit.h` has been included in `db_options.cc` is because we need two constants, `kUnknownChecksum` and `kUnknownChecksumFuncName`. We can put these two constants as `constexpr` in the public header `file_checksum.h`.
      Test plan (devserver):
      make check
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6952
      Reviewed By: zhichao-cao
      Differential Revision: D21925341
      Pulled By: riversand963
      fbshipit-source-id: 2902f3b74c97f0cf16c58ad24c095c787c3a40e2
  3. 06 Jun, 2020 8 commits
  4. 05 Jun, 2020 7 commits
    • Peter Dillinger's avatar
      Fix some defects reported by Coverity Scan (#6933) · aaece2a9
      Peter Dillinger authored
      Confusing checks for null that are never null
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6933
      Test Plan: make check
      Reviewed By: cheng-chang
      Differential Revision: D21885466
      Pulled By: pdillinger
      fbshipit-source-id: 4b48e03c2a33727f2702b0d12292f9fda5a3c475
    • Peter Dillinger's avatar
      Fix more defects reported by Coverity Scan (#6935) · c7432cc3
      Peter Dillinger authored
      Mostly uninitialized values: some probably written before use, but some seem like bugs. Also, destructor needs to be virtual, and possible use-after-free in test
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6935
      Test Plan: make check
      Reviewed By: siying
      Differential Revision: D21885484
      Pulled By: pdillinger
      fbshipit-source-id: e2e7cb0a0cf196f2b55edd16f0634e81f6cc8e08
    • Yanqin Jin's avatar
      Close file to avoid file-descriptor leakage (#6936) · a8170d77
      Yanqin Jin authored
      When operation on an open file descriptor fails, we should close the file descriptor.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6936
      Test Plan: make check
      Reviewed By: pdillinger
      Differential Revision: D21885458
      Pulled By: riversand963
      fbshipit-source-id: ba077a76b256a8537f21e22e4ec198f45390bf50
    • sdong's avatar
      Make StringAppendOperatorTest a parameterized test (#6930) · 6cbe9d97
      sdong authored
      StringAppendOperatorTest right now runs in a mode where RUN_ALL_TESTS() is executed twice for the same test but different settings. This creates a problem with a tool that expects every test to run once. Fix it by using a parameterized test instead.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6930
      Test Plan: Run the test and see it passed.
      Reviewed By: ltamasi
      Differential Revision: D21874145
      fbshipit-source-id: 55520b2d7f1ba9f3cba1e2d087fe86f43fb06145
    • sdong's avatar
      Fix ThreadLocalTest.SequentialReadWriteTest failure when running individually (#6929) · 31bd2d79
      sdong authored
      When running ThreadLocalTest.SequentialReadWriteTest individually, the test fails with:
      ] ./thread_local_test --gtest_filter="*SequentialReadWriteTest*"
      Note: Google Test filter = *SequentialReadWriteTest*
      [==========] Running 1 test from 1 test case.
      [----------] Global test environment set-up.
      [----------] 1 test from ThreadLocalTest
      [ RUN      ] ThreadLocalTest.SequentialReadWriteTest
      internal_repo_rocksdb/repo/util/thread_local_test.cc:144: Failure
            Expected: IDChecker::PeekId()
            Which is: 3
      To be equal to: base_id + 1u
            Which is: 2
      [  FAILED  ] ThreadLocalTest.SequentialReadWriteTest (1 ms)
      [----------] 1 test from ThreadLocalTest (1 ms total)
      [----------] Global test environment tear-down
      [==========] 1 test from 1 test case ran. (1 ms total)
      [  PASSED  ] 0 tests.
      [  FAILED  ] 1 test, listed below:
      [  FAILED  ] ThreadLocalTest.SequentialReadWriteTest
       1 FAILED TEST
      It appears that when running as the first test, PeakId() was updated twice. I didn't dig into it why but it doesn't seem to break the contract. Relax the assertion to make it pass.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6929
      Test Plan: Run the test individually and as the whole thread_local_test
      Reviewed By: riversand963
      Differential Revision: D21873999
      fbshipit-source-id: 1dcb6a2e9c38b6afd848027308bfe633342b7548
    • mrambacher's avatar
      Fix two core dumps when files are missing (#6922) · e85cbdb4
      mrambacher authored
      The LDB create and drop column family commands failed to check if theere was a valid database prior to dereferencing it, leading to a core dump.
      The SstFileDumper prefetch code would dereference a file when the file did not exist as part of the Prefetch code.  This dereference was moved inside an st.ok() check.
      Tests were added for both failure conditions.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6922
      Reviewed By: gg814
      Differential Revision: D21884024
      Pulled By: pdillinger
      fbshipit-source-id: bddd45c299aa9dc7e928c17a37a96521f8c9149e
    • sdong's avatar
      env_test */RunMany/* tests to run individually (#6931) · 0b45a68c
      sdong authored
      When run */RunMany/* tests individually, e.g. ChrootEnvWithDirectIO/EnvPosixTestWithParam.RunMany/0, they hang. It's because they insert to background thread pool without initializing them. Fix it.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6931
      Test Plan: Run ChrootEnvWithDirectIO/EnvPosixTestWithParam.RunMany/0 by itself and see it passes.
      Reviewed By: riversand963
      Differential Revision: D21875603
      fbshipit-source-id: 7f848174c1a660254a2b1f7e11cca5370793ba30
  5. 04 Jun, 2020 15 commits
    • Yanqin Jin's avatar
      Fix a typo (bug) when setting error during Flush (#6928) · 2f326183
      Yanqin Jin authored
      As title. The prior change to the line is a typo. Fixing it.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6928
      Test Plan: make check
      Reviewed By: zhichao-cao
      Differential Revision: D21873587
      Pulled By: riversand963
      fbshipit-source-id: f4837fc8792d7106bc230b7b499dfbb7a2847430
    • Zitan Chen's avatar
      API change: DB::OpenForReadOnly will not write to the file system unless... · 02df00d9
      Zitan Chen authored
      API change: DB::OpenForReadOnly will not write to the file system unless create_if_missing is true (#6900)
      DB::OpenForReadOnly will not write anything to the file system (i.e., create directories or files for the DB) unless create_if_missing is true.
      This change also fixes some subcommands of ldb, which write to the file system even if the purpose is for readonly.
      Two tests for this updated behavior of DB::OpenForReadOnly are also added.
      Other minor changes:
      1. Updated HISTORY.md to include this API change of DB::OpenForReadOnly;
      2. Updated the help information for the put and batchput subcommands of ldb with the option [--create_if_missing];
      3. Updated the comment of Env::DeleteDir to emphasize that it returns OK only if the directory to be deleted is empty.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6900
      Test Plan: passed make check; also manually tested a few ldb subcommands
      Reviewed By: pdillinger
      Differential Revision: D21822188
      Pulled By: gg814
      fbshipit-source-id: 604cc0f0d0326a937ee25a32cdc2b512f9a3be6e
    • sdong's avatar
      Make sure core components not depend on gtest (#6921) · 055b4d25
      sdong authored
      We recently removed the dependencies of core components on gtest. Add a Travis test to make sure it doesn't regress. Change cmake setting so that the gtest related components are only included when tests, benchmarks or stress tools are included in the build. Add this build setting in Travis to confirm it.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6921
      Test Plan: See Travis passes
      Reviewed By: ajkr
      Differential Revision: D21863564
      fbshipit-source-id: df26f50a8305a04ff19ffa8069a1857ecee10289
    • Stanislav Tkach's avatar
      Add (some) getters for options to the C API (#6925) · b7c825d5
      Stanislav Tkach authored
      Additionally I have extended the incomplete test added in the https://github.com/facebook/rocksdb/issues/6880.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6925
      Reviewed By: ajkr
      Differential Revision: D21869788
      Pulled By: pdillinger
      fbshipit-source-id: e9db80f259c57ca1bdcbc2c66cb938cb1ac26e48
    • sdong's avatar
      Revert "Update googletest from 1.8.1 to 1.10.0 (#6808)" (#6923) · afa35188
      sdong authored
      This reverts commit 8d87e9ce.
      Based on offline discussions, it's too early to upgrade to gtest 1.10, as it prevents some developers from using an older version of gtest to integrate to some other systems. Revert it for now.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6923
      Reviewed By: pdillinger
      Differential Revision: D21864799
      fbshipit-source-id: d0726b1ff649fc911b9378f1763316200bd363fc
    • Hans Holmberg's avatar
      Route GetTestDirectory to FileSystem in CompositeEnvWrappers (#6896) · 0f85d163
      Hans Holmberg authored
      GetTestDirectory implies a file system operation (it creates the
      default test directory if missing), so it should be routed to
      the FileSystem rather than the Env.
      Also remove the GetTestDirectory implementation in the PosixEnv,
      since it overrides GetTestDirectory in CompositeEnv making it
      impossible to override with a custom FileSystem.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6896
      Reviewed By: cheng-chang
      Differential Revision: D21868984
      Pulled By: ajkr
      fbshipit-source-id: e79bfef758d06dacef727c54b96abe62e78726fd
    • hfrt456's avatar
      fix IsDirectory function in env_hdfs.cc (#6917) · f005dac2
      hfrt456 authored
      fix IsDirectory function for hdfsEnv
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6917
      Reviewed By: cheng-chang
      Differential Revision: D21865020
      Pulled By: riversand963
      fbshipit-source-id: ad69ed564d027b7bbdf4c693dd57cd02622fb3f8
    • Levi Tamasi's avatar
      Mention the consistency check improvement in HISTORY.md (#6924) · 0b8c549b
      Levi Tamasi authored
      Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/6924
      Reviewed By: cheng-chang
      Differential Revision: D21865662
      Pulled By: ltamasi
      fbshipit-source-id: 83a01bcbb779cfba941154a36a9e735293a93211
    • Hao Chen's avatar
      correct level information in version_set.cc (#6920) · ffe08ffc
      Hao Chen authored
      fix these two issues https://github.com/facebook/rocksdb/issues/6912  and https://github.com/facebook/rocksdb/issues/6667
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6920
      Reviewed By: cheng-chang
      Differential Revision: D21864885
      Pulled By: ajkr
      fbshipit-source-id: 10e21fc1851b67a59d44358f59c64fa5523bd263
    • Anatoly Zhmur's avatar
      Add zstd_max_train_bytes to c interop (#6796) · 22e5c513
      Anatoly Zhmur authored
      Added setting of zstd_max_train_bytes compression option parameter to c interop.
      rocksdb_options_set_bottommost_compression_options was using bool parameter and thus not exported, updated it to unsigned char and added to c.h as well.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6796
      Reviewed By: cheng-chang
      Differential Revision: D21611471
      Pulled By: ajkr
      fbshipit-source-id: caaaf153de934837ad9af283c7f8c025ff0b0cf5
    • mrambacher's avatar
      Add OptionTypeInfo::Vector to parse/serialize vectors (#6424) · 0a17d953
      mrambacher authored
      The OptionTypeInfo::Vector method allows a vector<T> to be converted to/from strings via the options.
      The kVectorInt and kVectorCompressionType vectors were replaced with this methodology.
      As part of this change, the NextToken method was added to the OptionTypeInfo.  This method was refactored from code within the StringToMap function.
      Future types that could use this functionality include the EventListener vectors.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6424
      Reviewed By: cheng-chang
      Differential Revision: D21832368
      Pulled By: zhichao-cao
      fbshipit-source-id: e1ca766faff139d54e6e8407a9ec09ece6517439
    • Lucian Petrut's avatar
      Posix threads (#6865) · 172adce7
      Lucian Petrut authored
      Rocksdb is using the c++11 std::threads feature. The issue is that
      MINGW only supports it when using Posix threads.
      This change will allow rocksdb::port::WindowsThread to be replaced
      with std::thread, which in turn will allow Rocksdb to be cross
      compiled using MINGW.
      At the same time, we'll have to use GetCurrentProcessId instead of _getpid.
      Signed-off-by: default avatarLucian Petrut <lpetrut@cloudbasesolutions.com>
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6865
      Reviewed By: cheng-chang
      Differential Revision: D21864285
      Pulled By: ajkr
      fbshipit-source-id: 0982eed313e7d34d351b1364c1ccc722da473205
    • Peter Dillinger's avatar
      Some fixes for gcc 4.8 and add to Travis (#6915) · 43f8a9dc
      Peter Dillinger authored
      People keep breaking the gcc 4.8 compilation due to different
      warnings for shadowing member functions with locals. Adding to Travis
      to keep compatibility. (gcc 4.8 is default on CentOS 7.)
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6915
      Test Plan: local and Travis
      Reviewed By: siying
      Differential Revision: D21842894
      Pulled By: pdillinger
      fbshipit-source-id: bdcd4385127ee5d1cc222d87e53fb3695c32a9d4
    • Levi Tamasi's avatar
      Improve consistency checks in VersionBuilder (#6901) · 78e291b1
      Levi Tamasi authored
      The patch cleans up the code and improves the consistency checks around
      adding/deleting table files in `VersionBuilder`. Namely, it makes the checks
      stricter and improves them in the following ways:
      1) A table file can now only be deleted from the LSM tree using the level it
      resides on. Earlier, there was some unnecessary wiggle room for
      trivially moved files (they could be deleted using a lower level number than
      the actual one).
      2) A table file cannot be added to the tree if it is already present in the tree
      on any level (not just the target level). The earlier code only had an assertion
      (which is a no-op in release builds) that the newly added file is not already
      present on the target level.
      3) The above consistency checks around state transitions are now mandatory,
      as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op
      in release mode unless `force_consistency_checks` was set to `true`. The rationale
      here is that assuming that the initial state is consistent, a valid transition leads to a
      next state that is also consistent; however, an *invalid* transition offers no such
      guarantee. Hence it makes sense to validate the transitions unconditionally,
      and save `force_consistency_checks` for the paranoid checks that re-validate
      the entire state.
      4) The new checks build on the mechanism introduced in https://github.com/facebook/rocksdb/pull/6862,
      which enables us to efficiently look up the location (level and position within level)
      of files in a `Version` by file number. This makes the consistency checks much more
      efficient than the earlier `CheckConsistencyForDeletes`, which essentially
      performed a linear search.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6901
      Test Plan:
      Extended the unit tests and ran:
      `make check`
      `make whitebox_crash_test`
      Reviewed By: ajkr
      Differential Revision: D21822714
      Pulled By: ltamasi
      fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215
    • Peter Dillinger's avatar
      Fix handling of too-small filter partition size (#6905) · 9360776c
      Peter Dillinger authored
      Because ARM and some other platforms have a larger cache line
      size, they have a larger minimum filter size, which causes recently
      added PartitionedMultiGet test in db_bloom_filter_test to fail on those
      platforms. The code would actually end up using larger partitions,
      because keys_per_partition_ would be 0 and never == number of keys
      The code now attempts to get as close as possible to the small target
      size, while fully utilizing that filter size, if the target partition
      size is smaller than the minimum filter size.
      Also updated the test to break more uniformly across platforms
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6905
      Test Plan: updated test, tested on ARM
      Reviewed By: anand1976
      Differential Revision: D21840639
      Pulled By: pdillinger
      fbshipit-source-id: 11684b6d35f43d2e98b85ddb2c8dcfd59d670817
  6. 03 Jun, 2020 6 commits
    • Zhichao Cao's avatar
      Fix potential overflow of unsigned type in for loop (#6902) · 2adb7e37
      Zhichao Cao authored
      x.size() -1 or y - 1 can overflow to an extremely large value when x.size() pr y is 0 when they are unsigned type. The end condition of i in the for loop will be extremely large, potentially causes segment fault. Fix them.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6902
      Test Plan: pass make asan_check
      Reviewed By: ajkr
      Differential Revision: D21843767
      Pulled By: zhichao-cao
      fbshipit-source-id: 5b8b88155ac5a93d86246d832e89905a783bb5a1
    • Zhichao Cao's avatar
      Replace Status with IOStatus in CopyFile and CreateFile (#6916) · 556972e9
      Zhichao Cao authored
      Replace Status with IOStatus in CopyFile and CreateFile.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6916
      Test Plan: pass make asan_check
      Reviewed By: cheng-chang
      Differential Revision: D21843775
      Pulled By: zhichao-cao
      fbshipit-source-id: 524d4a0fcf47f0941b923da0346e0de71607f5f6
    • sdong's avatar
      Remove gtest dependency in non-test code under utilities/cassandra (#6908) · bfc9737a
      sdong authored
      production code under utilities/cassandra depends on gtest.h. Remove them.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6908
      Test Plan: Run all existing tests.
      Reviewed By: ajkr
      Differential Revision: D21842606
      fbshipit-source-id: a098e0b49c9aeac51cc90a79562ad9897a36122c
    • Stanislav Tkach's avatar
      Expose rocksdb_options_copy function to the C API (#6880) · 38f988d3
      Stanislav Tkach authored
      Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/6880
      Reviewed By: ajkr
      Differential Revision: D21842752
      Pulled By: pdillinger
      fbshipit-source-id: eda326f551ddd9cb397681544b9e9799ea614e52
    • Peter Dillinger's avatar
      For ApproximateSizes, pro-rate table metadata size over data blocks (#6784) · 14eca6bf
      Peter Dillinger authored
      The implementation of GetApproximateSizes was inconsistent in
      its treatment of the size of non-data blocks of SST files, sometimes
      including and sometimes now. This was at its worst with large portion
      of table file used by filters and querying a small range that crossed
      a table boundary: the size estimate would include large filter size.
      It's conceivable that someone might want only to know the size in terms
      of data blocks, but I believe that's unlikely enough to ignore for now.
      Similarly, there's no evidence the internal function AppoximateOffsetOf
      is used for anything other than a one-sided ApproximateSize, so I intend
      to refactor to remove redundancy in a follow-up commit.
      So to fix this, GetApproximateSizes (and implementation details
      ApproximateSize and ApproximateOffsetOf) now consistently include in
      their returned sizes a portion of table file metadata (incl filters
      and indexes) based on the size portion of the data blocks in range. In
      other words, if a key range covers data blocks that are X% by size of all
      the table's data blocks, returned approximate size is X% of the total
      file size. It would technically be more accurate to attribute metadata
      based on number of keys, but that's not computationally efficient with
      data available and rarely a meaningful difference.
      Also includes miscellaneous comment improvements / clarifications.
      Also included is a new approximatesizerandom benchmark for db_bench.
      No significant performance difference seen with this change, whether ~700 ops/sec with cache_index_and_filter_blocks and small cache or ~150k ops/sec without cache_index_and_filter_blocks.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6784
      Test Plan:
      Test added to DBTest.ApproximateSizesFilesWithErrorMargin.
      Old code running new test...
          [ RUN      ] DBTest.ApproximateSizesFilesWithErrorMargin
          db/db_test.cc:1562: Failure
          Expected: (size) <= (11 * 100), actual: 9478 vs 1100
      Other tests updated to reflect consistent accounting of metadata.
      Reviewed By: siying
      Differential Revision: D21334706
      Pulled By: pdillinger
      fbshipit-source-id: 6f86870e45213334fedbe9c73b4ebb1d8d611185
    • sdong's avatar
      Reduce dependency on gtest dependency in release code (#6907) · 298b00a3
      sdong authored
      Release code now depends on gtest, indirectly through including "test_util/testharness.h". This creates multiple problems. One important reason is the definition of IGNORE_STATUS_IF_ERROR() in test_util/testharness.h. Move it to sync_point.h instead.
      Note that utilities/cassandra/format.h still depends on "test_util/testharness.h". This will be resolved in a separate diff.
      Pull Request resolved: https://github.com/facebook/rocksdb/pull/6907
      Test Plan: Run all existing tests.
      Reviewed By: ajkr
      Differential Revision: D21829884
      fbshipit-source-id: 9253c19ffde2936f3ae68998210f8e54f645a6e6
  7. 02 Jun, 2020 2 commits