Commit Graph

8896 Commits

Author SHA1 Message Date
wolfkdy
29e24434fe refine code (#6420)
Summary:
I create a new branch from the branch new upsteram/master and "git merge --squash".
Maybe it will fix everything.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6420

Differential Revision: D19897152

Pulled By: zhichao-cao

fbshipit-source-id: 6575d9e3b23e360f42ee1480b43028b5fcc20136
2020-02-13 18:55:02 -08:00
Manuel Ung
908b1ee64e WriteUnPrepared: Fix assertion during recovery (#6419)
Summary:
During recovery, multiple (un)prepared batches could exist in the same WAL record due to group commit. This breaks an assertion in `MemTableInserter::MarkBeginPrepare`.

To fix, reset unprepared_batch_ to false after `MarkEndPrepare`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6419

Differential Revision: D19896148

Pulled By: lth

fbshipit-source-id: b1a32ef88f775a0881264a18bd1a4a5b8c85eee3
2020-02-13 18:52:05 -08:00
Manuel Ung
fb571509a7 WriteUnPrepared: Enable WAL during crash recovery (#6418)
Summary:
Unfortunately, it seems like mysqld reuses xids across machine restarts. When that happens, we could have something like the following happening:

```
BEGIN_PREPARE(unprepared) Put(a) END_PREPARE(xid = 1)
-- crash and recover with Put(a) rolled back as it was not prepared
BEGIN_PREPARE(prepared) Put(b) END_PREPARE(xid = 1)
COMMIT(xid = 1)
-- crash and recover with both a, b
```

To solve this, we will have to log the rollback batch into the WAL during recovery.

WritePrepared already logs the rollback batch into the WAL, if a rollback happens after prepare, so there is no problem there.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6418

Differential Revision: D19896151

Pulled By: lth

fbshipit-source-id: 2ff65ddc5fe75efd57736fed4b7cd7a109d26609
2020-02-13 18:44:39 -08:00
sdong
ac8e89a443 Should flush and sync WAL when writing it in DB::Open() (#6417)
Summary:
A recent fix related to 2pc https://github.com/facebook/rocksdb/pull/6313/ writes something to WAL, but does not flush or sync. This causes assertion failure "impl->TEST_WALBufferIsEmpty()" if manual_wal_flush = true. We should fsync the entry to make sure a second power reset can recover.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6417

Test Plan: Add manual_wal_flush=true case in TransactionTest.DoubleCrashInRecovery and fix a bug in the test so that the bug can be reproduced. It passes with the fix.

Differential Revision: D19894537

fbshipit-source-id: f1e84e49e2269f583c6019743118292cd8b6598e
2020-02-13 18:41:04 -08:00
Cheng Chang
46516778dd Fix flaky test DecreaseNumBgThreads (#6393)
Summary:
The DecreaseNumBgThreads test keeps failing on Windows in AppVeyor.
It fails because it depends on a timed wait for the tasks to be dequeued from the threadpool's internal queue, but within the specified time, the task might have not been scheduled onto the newly created threads.
https://github.com/facebook/rocksdb/pull/6232 tries to fix this by waiting for longer time to let the threads scheduled.
This PR tries to fix this by replacing the timed wait with a synchronization on the task's internal conditional variable.
When the number of threads increases, instead of guessing the time needed for the task to be scheduled, it directly blocks on the conditional variable until the task starts running.
But when thread number is reduced, it still does a timed wait, but this does not lead to the flakiness now, will try to remove these timed waits in a future PR.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6393

Test Plan: Wait to see whether AppVeyor tests pass.

Differential Revision: D19890928

Pulled By: cheng-chang

fbshipit-source-id: 4e56e4addf625c98c0876e62d9d57a6f0a156f76
2020-02-13 17:27:18 -08:00
sdong
acfee40af5 Remove IO URING compiler flags (#6415)
Summary:
Since IO Uring feature is not stable. Remove it from buck configuration.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6415

Test Plan: See internal build pass

Differential Revision: D19892988

fbshipit-source-id: 7fc01efc2af5ed707fb8e4e4674223aeb83cd5ea
2020-02-13 16:51:10 -08:00
Huisheng Liu
5138764eb5 Fix destroydb (#6308)
Summary:
It's observed on Windows DestroyDB failed to remove the log file because the logger is still alive in sst file manager and holding a handle to the log file. This fix makes sure the logger is released before attempt to clear the database directory.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6308

Differential Revision: D19818829

Pulled By: riversand963

fbshipit-source-id: 54c3e6859aadaaba4a49b3e851b73dc35ec7dc6a
2020-02-13 11:21:27 -08:00
sdong
df3f33dd05 Fix db_bench LITE build recently broken (#6411)
Summary:
A recent change https://github.com/facebook/rocksdb/pull/6386 broke LITE build in a trivial way. Fix it.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6411

Test Plan: Run "LITE=1 make all"

Differential Revision: D19871765

fbshipit-source-id: 74f0ad3f8a9d666fbde0da7fd29ba1547a811f77
2020-02-13 10:52:50 -08:00
Cheng Chang
a676001f95 Revert usage of Defer. (#6410)
Summary:
Seems like this caused the following test failure on AppVeyor:
DBTest2.CrashInRecoveryMultipleCF
c:\projects\rocksdb\db\db_test_util.cc(107): error: DestroyDB(dbname_, options)
IO error: Failed to delete: C:\projects\rocksdb\db_tests\\testrocksdb-3112//db_test2_10791409581227174103/000013.sst: Access is denied.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6410

Test Plan: Wait to see whether the AppVeyor test passes.

Differential Revision: D19879872

Pulled By: cheng-chang

fbshipit-source-id: 59a9c55ca88566e9210c0b715ecc45a4fd9afe26
2020-02-13 10:31:32 -08:00
sdong
6e97d4de00 By default turn IO Uring off. (#6405)
Summary:
We realized bugs related to IO Uring. Turn it off by default.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6405

Test Plan: Manually run build_tools/build_detect_platform and observe outputs.

Differential Revision: D19862792

fbshipit-source-id: 5d5e8e2762997b72a145ae59389ef3d7e4ccd060
2020-02-12 18:01:49 -08:00
Burton Li
e64508917b db_bench supports for generating random variable sized value. (#6386)
Summary:
1. `db_bench` now supports `value_size_distribution_type`, `value_size_min`, `value_size_max` options for generating random variable sized value.
2. Added `blob_db_compression_type` option for BlobDB to enable blob compression.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6386

Differential Revision: D19859406

Pulled By: zhichao-cao

fbshipit-source-id: ace52674090023fde15d832392110bf288a8e215
2020-02-12 14:47:03 -08:00
anand76
3e49249d30 Ensure all MultiGet IO errors are propagated to user (#6403)
Summary:
Unrevert the previous fix to propagate error status, and an additional fix to not treat a memtable lookup MergeInProgress status as an error.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6403

Test Plan:
Unit tests
Tried running stress tests but couldn't repro the stress failure

Differential Revision: D19846721

Pulled By: anand1976

fbshipit-source-id: 7db10cccbdc863d9b559497f0a46b608d2488ca4
2020-02-11 17:27:22 -08:00
Tomas Kolda
e412a426d6 JNI direct buffer support for basic operations (#2283)
Summary:
It is very useful to support direct ByteBuffers in Java. It allows to have zero memory copy and some serializers are using that directly so one do not need to create byte[] array for it.

This change also contains some fixes for Windows JNI build.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/2283

Differential Revision: D19834971

Pulled By: pdillinger

fbshipit-source-id: 44173aa02afc9836c5498c592fd1ea95b6086e8e
2020-02-11 14:48:30 -08:00
Yanqin Jin
28aa09dcce Pass info_log_level to the inner logger of AutoRollLogger (#6388)
Summary:
Before this fix, the info_log_level passed from CreateLoggerFromOptions() will
be ignored by AutoRollLogger::logger_. This PR fixes it by setting the info log
level of logger_ during ResetLogger().

Test plan (dev server):
```
COMPILE_WITH_TSAN=1 make all && make check
make all && make check
```
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6388

Differential Revision: D19828045

Pulled By: riversand963

fbshipit-source-id: e1ac7de3a2090bee53b6c667f71a11f1774163e6
2020-02-10 22:26:08 -08:00
anand76
35ed530d2c Revert "Check KeyContext status in MultiGet (#6387)" (#6401)
Summary:
This reverts commit d70011bccc. The commit is causing some stress test failure due to unexpected Status::MergeInProgress() return for some keys.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6401

Differential Revision: D19826623

Pulled By: anand1976

fbshipit-source-id: edd634cede9cb7bdd2cb8f46e662ea709b16d2f1
2020-02-10 22:23:36 -08:00
Cheng Chang
d3ba398bbd Update unit tests for PinnableSlice (#6399)
Summary:
1. remove AssertEmpty because calling methods on moved objects is discouraged.
2. add a test to assert that the internal buffer is moved instead of being copied.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6399

Test Plan:
make slice_test && ./slice_test
USE_CLANG=1 make analyze

Differential Revision: D19825372

Pulled By: cheng-chang

fbshipit-source-id: 2e26f8ce5ec3edbfce067db045e80bd433e704f4
2020-02-10 18:13:27 -08:00
Cheng Chang
dafb568052 Add utility class Defer (#6382)
Summary:
Add a utility class `Defer` to defer the execution of a function until the Defer object goes out of scope.
Used in VersionSet:: ProcessManifestWrites as an example.
The inline comments for class `Defer` have more details.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6382

Test Plan: `make defer_test version_set_test && ./defer_test && ./version_set_test`

Differential Revision: D19797538

Pulled By: cheng-chang

fbshipit-source-id: b1a9b7306e4fd4f48ec2ab55783caa561a315f0f
2020-02-10 17:59:47 -08:00
Levi Tamasi
cbf5f3be43 Do not move VersionEdit into AtomicGroupReadBuffer (#6400)
Summary:
https://github.com/facebook/rocksdb/pull/6383 surfaced an issue with
`VersionSet`/`ReactiveVersionSet` and `AtomicGroupReadBuffer::AddEdit`
(which was added in https://github.com/facebook/rocksdb/pull/5411):
`AddEdit` moves the `VersionEdit` passed to it into `replay_buffer_`,
however, the client `VersionSet` classes keep using it afterwards. This
*seemed to* work before the refactoring but it really did not: since
`VersionEdit` used to have a user-declared destructor, no move
constructor/move assignment operator was generated, and the `move` in
`AddEdit` was really a copy. The patch makes the copy explicit. Note: it
should be possible to rework this logic so that we can get away
with the move but for now, this should fix the issue.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6400

Test Plan:
`make check`
`make analyze`

Differential Revision: D19824466

Pulled By: ltamasi

fbshipit-source-id: f38033967daf2a39c78dcd6e12978bafe37632b4
2020-02-10 17:15:42 -08:00
Zhichao Cao
4369f2c7bb Checksum for each SST file and stores in MANIFEST (#6216)
Summary:
In the current code base, RocksDB generate the checksum for each block and verify the checksum at usage. Current PR enable SST file checksum. After a SST file is generated by Flush or Compaction, RocksDB generate the SST file checksum and store the checksum value and checksum method name in the vs_info and MANIFEST as part for the FileMetadata.

Added the enable_sst_file_checksum to Options to enable or disable file checksum. Added sst_file_checksum to Options such that user can plugin their own SST file checksum calculate method via overriding the SstFileChecksum class. The checksum information inlcuding uint32_t checksum value and a checksum name (string).  A new tool is added to LDB such that user can dump out a list of file checksum information from MANIFEST. If user enables the file checksum but does not provide the sst_file_checksum instance, RocksDB will use the default crc32checksum implemented in table/sst_file_checksum_crc32c.h
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6216

Test Plan: Added the testing case in table_test and ldb_cmd_test to verify checksum is correct in different level. Pass make asan_check.

Differential Revision: D19171461

Pulled By: zhichao-cao

fbshipit-source-id: b2e53479eefc5bb0437189eaa1941670e5ba8b87
2020-02-10 15:52:52 -08:00
sdong
594e815e32 Make clang analyze happy with options_test (#6398)
Summary:
clang analysis shows following warning:

options/options_test.cc:1554:24: warning: The left operand of '-' is a garbage value
            (file_size - 1) / readahead_size + 1);
             ~~~~~~~~~ ^

Explicitly initialize file_size and add an assertion to make clang analysis happy.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6398

Test Plan: Run "make analysis" and see the warning goes away.

Differential Revision: D19819662

fbshipit-source-id: 1589ea91c0c8f78242538f01448e4ad0e5fbc219
2020-02-10 15:50:25 -08:00
sdong
b2bc1da561 Try to fix some analysis failures
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/6384

Test Plan: Wait and see the analysis result.

Differential Revision: D19781072

fbshipit-source-id: 75e7cb6ee619ebd289841eaabea03dd075c09d3b
2020-02-10 13:37:14 -08:00
Yutian Li
2e0159ec9e Add error status for no_slowdown & low priority write (#6396)
Summary:
When `no_slowdown` is enabled, it returns `Status::Incomplete("Write stall")` if a stall would occur. This patch adds descriptive text for when `no_slowdown` and `low_pri` are enabled.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6396

Differential Revision: D19808978

Pulled By: cheng-chang

fbshipit-source-id: a53b0d25ed414c821a086531e0222027f925e627
2020-02-10 12:33:16 -08:00
Kefu Chai
debc4ef18b utilities/env_librados: copy use bufferlist::iterator (#6395)
Summary:
to adapt the change in ceph upstream where the bufferlist::copy() method
was removed in
c724369010

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6395

Differential Revision: D19816815

Pulled By: zhichao-cao

fbshipit-source-id: 9210767b91af0ecdcf5dfaa3e70edcaeea55135f
2020-02-10 11:31:16 -08:00
blackyblack
84b41a6969 Only add gtest target when building with tests (#6377)
Summary:
External CMAKE projects cannot add own gtest target due to CMP0002 policy. We should not add gtest target unless we build with tests.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6377

Differential Revision: D19801708

fbshipit-source-id: 662484683e8737e10397ddb0d17705da5243c4f9
2020-02-07 17:16:44 -08:00
anand76
d70011bccc Check KeyContext status in MultiGet (#6387)
Summary:
Currently, any IO errors and checksum mismatches while reading data
blocks, are being ignored by the batched MultiGet. Its only looking at
the GetContext state. Fix that.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6387

Test Plan: Add unit tests

Differential Revision: D19799819

Pulled By: anand1976

fbshipit-source-id: 46133dccbb04e64067b9fe6cda73e282203db969
2020-02-07 16:48:16 -08:00
Robert Yang
4e457278fa db/write_thread.cc: Initialize state (#6275)
Summary:
Fixed an error when compiled with -Og:
db/write_thread.cc:183:14: error: 'state' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6275

Differential Revision: D19381755

fbshipit-source-id: a90bf3cd4a7248d9d71219e918fc6253deb97e3c
2020-02-07 15:20:38 -08:00
sdong
876c2dbff4 Allow readahead when reading option files. (#6372)
Summary:
Right, when reading from option files, no readahead is used and 8KB buffer is used. It might introduce high latency if the file system provide high latency and doesn't do readahead. Instead, introduce a readahead to the file. When calling inside DB, infer the value from options.log_readahead. Otherwise, a default 512KB readahead size is used.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6372

Test Plan: Add --log_readahead_size in db_bench. Run it with several options and observe read size from option files using strace.

Differential Revision: D19727739

fbshipit-source-id: e6d8053b0a64259abc087f1f388b9cd66fa8a583
2020-02-07 15:18:26 -08:00
Cheng Chang
b42fa1497f Support move semantics for PinnableSlice (#6374)
Summary:
It's logically correct for PinnableSlice to support move semantics to transfer ownership of the pinned memory region. This PR adds both move constructor and move assignment to PinnableSlice.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6374

Test Plan:
A set of unit tests for the move semantics are added in slice_test.
So `make slice_test && ./slice_test`.

Differential Revision: D19739254

Pulled By: cheng-chang

fbshipit-source-id: f898bd811bb05b2d87384ec58b645e9915e8e0b1
2020-02-07 14:26:26 -08:00
Chad Austin
25fbdc5a31 Fix Buck build on macOS (#6378)
Summary:
liburing is a Linux-specific dependency, so make sure it's configured in the Linux-only Buck rules.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6378

Test Plan:
```
~/fbcode $ cp internal_repo_rocksdb/repo/TARGETS rocksdb/src
~/fbcode $ buck build mode/mac eden
```

Reviewed By: chadaustin

Differential Revision: D19760039

Pulled By: riversand963

fbshipit-source-id: 2abfce81c8b17965ef76012262cd117708e0294f
2020-02-07 14:20:12 -08:00
Levi Tamasi
752c87af78 Clean up VersionEdit a bit (#6383)
Summary:
This is a bunch of small improvements to `VersionEdit`. Namely, the patch

* Makes the names and order of variables, methods, and code chunks related
  to the various information elements more consistent, and adds missing
  getters for the sake of completeness.
* Initializes previously uninitialized stack variables.
* Marks all getters const to improve const correctness.
* Adds in-class initializers and removes the default ctor that would
  create an object with uninitialized built-in fields and call `Clear`
  afterwards.
* Adds a new type alias for new files and changes the existing `typedef`
  for deleted files into a type alias as well.
* Makes the helper method `DecodeNewFile4From` private.
* Switches from long-winded iterator syntax to range based loops in a
  couple of places.
* Fixes a couple of assignments where an integer 0 was assigned to
  boolean members.
* Fixes a getter which used to return a `const std::string` instead of
the intended `const std::string&`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6383

Test Plan: make check

Differential Revision: D19780537

Pulled By: ltamasi

fbshipit-source-id: b0b4f09fee0ec0e7c7b7a6d76bfe5346e91824d0
2020-02-07 13:27:06 -08:00
Cheng Chang
5f478b9f75 Remove outdated comment (#6379)
Summary:
Since the logic for handling IDENTITY file is now inside `NewDB`, the comment above `NewDB` is no longer relevant.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6379

Test Plan: not needed

Differential Revision: D19795440

Pulled By: cheng-chang

fbshipit-source-id: 0b1cca87ac6d92474701c46aa4c8d4d708bfa19b
2020-02-07 13:18:43 -08:00
Levi Tamasi
1b4be4cac9 BlobDB: ignore trivially moved files when updating the SST<->blob file mapping (#6381)
Summary:
BlobDB keeps track of the mapping between SSTs and blob files using
the `OnFlushCompleted` and `OnCompactionCompleted` callbacks of
the `EventListener` interface: upon receiving a flush notification, a link
is added between the newly flushed SST and the corresponding blob file;
for compactions, links are removed for the inputs and added for the outputs.
The earlier code performed this link deletion and addition even for
trivially moved files; the new code walks through the two lists together
(in a fashion that's similar to merge sort) and skips such files.
This should mitigate https://github.com/facebook/rocksdb/issues/6338,
wherein an assertion is triggered with the earlier code when a compaction
notification for a trivial move precedes the flush notification for the
moved SST.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6381

Test Plan: make check

Differential Revision: D19773729

Pulled By: ltamasi

fbshipit-source-id: ae0f273ded061110dd9334e8fb99b0d7786650b0
2020-02-07 12:50:57 -08:00
Cheng Chang
107a7ca930 Remove inappropriate comments (#6371)
Summary:
The comments are for iterators, not Cleanable.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6371

Test Plan: no need

Differential Revision: D19727527

Pulled By: cheng-chang

fbshipit-source-id: c74aeffa27ea0ce15a36ff6f9694826712cd1c70
2020-02-07 12:35:24 -08:00
Cheng Chang
0a74e1b958 Add status checks during DB::Open (#6380)
Summary:
Several statuses were not checked during DB::Open.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6380

Test Plan: make check

Differential Revision: D19780237

Pulled By: cheng-chang

fbshipit-source-id: c8d189d20344bd1607890dd1449345bda2ef96b9
2020-02-07 12:32:09 -08:00
Yanqin Jin
f361cedf06 Atomic flush rollback once on failure (#6385)
Summary:
Before this fix, atomic flush codepath may hit an assertion failure on a specific failure case.
If all flush jobs within an atomic flush succeed (they do not write to MANIFEST), but batch writing version edits to MANIFEST fails, then `cfd->imm()->RollbackMemTableFlush()` will be called twice, and the second invocation hits assertion failure `assert(m->flush_in_progress_)` since the first invocation resets the variable `flush_in_progress_` to false already.

Test plan (dev server):
```
./db_flush_test --gtest_filter=DBAtomicFlushTest/DBAtomicFlushTest.RollbackAfterFailToInstallResults
make check
```
Both must succeed.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6385

Differential Revision: D19782943

Pulled By: riversand963

fbshipit-source-id: 84e1592625e729d1b70fdc8479959387a74cb121
2020-02-07 10:52:10 -08:00
atul
c6f75516b7 Fixing the documentation of the function (#4803)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/6354

Differential Revision: D19725459

Pulled By: riversand963

fbshipit-source-id: fded24576251bfa4b289399f0909f1fe43426e28
2020-02-06 10:30:44 -08:00
Cheng Chang
f5f79f01a2 Be able to read compatible leveldb sst files (#6370)
Summary:
In `DBSSTTest.SSTsWithLdbSuffixHandling`, some sst files are renamed to ldb files, the original intention of the test is to test that the ldb files can be loaded along with the sst files.

The original test checks this by `ASSERT_NE("NOT_FOUND", Get(Key(k)))`, but the problem is `Get(Key(k))` returns IO error due to path not found instead of NOT_FOUND, so the success of ASSERT_NE does not mean the key can be retrieved.

This PR updates the test to make sure Get(Key(k)) returns the original value.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6370

Test Plan: make db_sst_test && ./db_sst_test

Differential Revision: D19726278

Pulled By: cheng-chang

fbshipit-source-id: 993127f56457b315e669af4eeb92d6f956b7a4b7
2020-02-06 10:15:44 -08:00
sdong
24c9dce825 Remove include math.h (#6373)
Summary:
We see some odd errors complaining math. However, it doesn't seem that it is needed to be included. Remove the include of math.h. Just removing it from db_bench doesn't seem to break anything. Replacing sqrt from std::sqrt seems to work for histogram.cc
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6373

Test Plan: Watch Travis and appveyor to run.

Differential Revision: D19730068

fbshipit-source-id: d3ad41defcdd9f51c2da1a3673fb258f5dfacf47
2020-02-05 21:00:49 -08:00
Mike Kolupaev
1ed7d9b1b5 Avoid lots of calls to Env::GetFileSize() in SstFileManagerImpl when opening DB (#6363)
Summary:
Before this PR it calls GetFileSize() once for each sst file in the DB. This can take a long time if there are be tens of thousands of sst files (e.g. in thousands of column families), and even longer if Env is talking to some remote service rather than local filesystem. This PR makes DB::Open() use sst file sizes that are already known from manifest (typically almost all files in the DB) and only call GetFileSize() for non-sst or obsolete files. Note that GetFileSize() is also called and checked against manifest in CheckConsistency(), so the calls in SstFileManagerImpl were completely redundant.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6363

Test Plan: deployed to a test cluster, looked at a dump of Env calls (from a custom instrumented Env) - no more thousands of GetFileSize()s.

Differential Revision: D19702509

Pulled By: al13n321

fbshipit-source-id: 99f8110620cb2e9d0c092dfcdbb11f3af4ff8b73
2020-02-04 13:41:53 -08:00
sdong
3a073234da Consolidate ReadFileToString() (#6366)
Summary:
It's a minor refactoring. We have two ReadFileToString() but they are very similar. Make the one with Env argument calls the one with FS argument instead.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6366

Test Plan: Run all existing tests

Differential Revision: D19712332

fbshipit-source-id: 5ae6fabf6355938690d95cda52afd1f39e0a7823
2020-02-04 11:39:23 -08:00
sdong
69c8614815 Avoid to get manifest file size when recovering from it. (#6369)
Summary:
Right now RocksDB gets manifest file size before recovering from it. The information is available in LogReader. Use it instead to prevent one file system call.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6369

Test Plan: Run all existing tests

Differential Revision: D19714872

fbshipit-source-id: 0144be324d403c99e3da875ea2feccc8f64e883d
2020-02-04 11:39:23 -08:00
Mike Kolupaev
637e64b9ac Add an option to prevent DB::Open() from querying sizes of all sst files (#6353)
Summary:
When paranoid_checks is on, DBImpl::CheckConsistency() iterates over all sst files and calls Env::GetFileSize() for each of them. As far as I could understand, this is pretty arbitrary and doesn't affect correctness - if filesystem doesn't corrupt fsynced files, the file sizes will always match; if it does, it may as well corrupt contents as well as sizes, and rocksdb doesn't check contents on open.

If there are thousands of sst files, getting all their sizes takes a while. If, on top of that, Env is overridden to use some remote storage instead of local filesystem, it can be *really* slow and overload the remote storage service. This PR adds an option to not do GetFileSize(); instead it does GetChildren() for parent directory to check that all the expected sst files are at least present, but doesn't check their sizes.

We can't just disable paranoid_checks instead because paranoid_checks do a few other important things: make the DB read-only on write errors, print error messages on read errors, etc.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6353

Test Plan: ran the added sanity check unit test. Will try it out in a LogDevice test cluster where the GetFileSize() calls are causing a lot of trouble.

Differential Revision: D19656425

Pulled By: al13n321

fbshipit-source-id: c2c421b367633033760d1f56747bad206d1fbf82
2020-02-04 01:27:26 -08:00
anand76
7330ec0ff1 Fix a test failure in error_handler_test (#6367)
Summary:
Fix an intermittent failure in
DBErrorHandlingTest.CompactionManifestWriteError due to a race between
background error recovery and the main test thread calling
TEST_WaitForCompact().
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6367

Test Plan: Run the test using gtest_parallel

Differential Revision: D19713802

Pulled By: anand1976

fbshipit-source-id: 29e35dc26e0984fe8334c083e059f4fa1f335d68
2020-02-03 18:16:52 -08:00
sdong
f195d8d523 Use ReadFileToString() to get content from IDENTITY file (#6365)
Summary:
Right now when reading IDENTITY file, we use a very similar logic as ReadFileToString() while it does an extra file size check, which may be expensive in some file systems. There is no reason to duplicate the logic. Use ReadFileToString() instead.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6365

Test Plan: RUn all existing tests.

Differential Revision: D19709399

fbshipit-source-id: 3bac31f3b2471f98a0d2694278b41e9cd34040fe
2020-02-03 17:40:49 -08:00
sdong
36c504be17 Avoid create directory for every column families (#6358)
Summary:
A relatively recent regression causes for every CF, create and open directory is called for the DB directory, unless CF has a private directory. This doesn't scale well with large number of column families.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6358

Test Plan: Run all existing tests and see it pass. strace with db_bench --num_column_families and observe it doesn't open directory for number of column families.

Differential Revision: D19675141

fbshipit-source-id: da01d9216f1dae3f03d4064fbd88ce71245bd9be
2020-02-03 14:13:39 -08:00
Huisheng Liu
eb4d6af5ae Error handler test fix (#6266)
Summary:
MultiDBCompactionError fails when it verifies the number of files on level 0 and level 1 without waiting for compaction to finish.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6266

Differential Revision: D19701639

Pulled By: riversand963

fbshipit-source-id: e96d511bcde705075f073e0b550cebcd2ecfccdc
2020-02-03 13:32:53 -08:00
Adam Retter
7242dae7fe Improve RocksJava Comparator (#6252)
Summary:
This is a redesign of the API for RocksJava comparators with the aim of improving performance. It also simplifies the class hierarchy.

**NOTE**: This breaks backwards compatibility for existing 3rd party Comparators implemented in Java... so we need to consider carefully which release branches this goes into.

Previously when implementing a comparator in Java the developer had a choice of subclassing either `DirectComparator` or `Comparator` which would use direct and non-direct byte-buffers resepectively (via `DirectSlice` and `Slice`).

In this redesign there we have eliminated the overhead of using the Java Slice classes, and just use `ByteBuffer`s. The `ComparatorOptions` supplied when constructing a Comparator allow you to choose between direct and non-direct byte buffers by setting `useDirect`.

In addition, the `ComparatorOptions` now allow you to choose whether a ByteBuffer is reused over multiple comparator calls, by setting `maxReusedBufferSize > 0`. When buffers are reused, ComparatorOptions provides a choice of mutex type by setting `useAdaptiveMutex`.

 ---
[JMH benchmarks previously indicated](https://github.com/facebook/rocksdb/pull/6241#issue-356398306) that the difference between C++ and Java for implementing a comparator was ~7x slowdown in Java.

With these changes, when reusing buffers and guarding access to them via mutexes the slowdown is approximately the same. However, these changes offer a new facility to not reuse mutextes, which reduces the slowdown to ~5.5x in Java. We also offer a `thread_local` mechanism for reusing buffers, which reduces slowdown to ~5.2x in Java (closes https://github.com/facebook/rocksdb/pull/4425).

These changes also form a good base for further optimisation work such as further JNI lookup caching, and JNI critical.

 ---
These numbers were captured without jemalloc. With jemalloc, the performance improves for all tests, and the Java slowdown reduces to between 4.8x and 5.x.

```
ComparatorBenchmarks.put                                                native_bytewise  thrpt   25  124483.795 ± 2032.443  ops/s
ComparatorBenchmarks.put                                        native_reverse_bytewise  thrpt   25  114414.536 ± 3486.156  ops/s
ComparatorBenchmarks.put              java_bytewise_non-direct_reused-64_adaptive-mutex  thrpt   25   17228.250 ± 1288.546  ops/s
ComparatorBenchmarks.put          java_bytewise_non-direct_reused-64_non-adaptive-mutex  thrpt   25   16035.865 ± 1248.099  ops/s
ComparatorBenchmarks.put                java_bytewise_non-direct_reused-64_thread-local  thrpt   25   21571.500 ±  871.521  ops/s
ComparatorBenchmarks.put                  java_bytewise_direct_reused-64_adaptive-mutex  thrpt   25   23613.773 ± 8465.660  ops/s
ComparatorBenchmarks.put              java_bytewise_direct_reused-64_non-adaptive-mutex  thrpt   25   16768.172 ± 5618.489  ops/s
ComparatorBenchmarks.put                    java_bytewise_direct_reused-64_thread-local  thrpt   25   23921.164 ± 8734.742  ops/s
ComparatorBenchmarks.put                              java_bytewise_non-direct_no-reuse  thrpt   25   17899.684 ±  839.679  ops/s
ComparatorBenchmarks.put                                  java_bytewise_direct_no-reuse  thrpt   25   22148.316 ± 1215.527  ops/s
ComparatorBenchmarks.put      java_reverse_bytewise_non-direct_reused-64_adaptive-mutex  thrpt   25   11311.126 ±  820.602  ops/s
ComparatorBenchmarks.put  java_reverse_bytewise_non-direct_reused-64_non-adaptive-mutex  thrpt   25   11421.311 ±  807.210  ops/s
ComparatorBenchmarks.put        java_reverse_bytewise_non-direct_reused-64_thread-local  thrpt   25   11554.005 ±  960.556  ops/s
ComparatorBenchmarks.put          java_reverse_bytewise_direct_reused-64_adaptive-mutex  thrpt   25   22960.523 ± 1673.421  ops/s
ComparatorBenchmarks.put      java_reverse_bytewise_direct_reused-64_non-adaptive-mutex  thrpt   25   18293.317 ± 1434.601  ops/s
ComparatorBenchmarks.put            java_reverse_bytewise_direct_reused-64_thread-local  thrpt   25   24479.361 ± 2157.306  ops/s
ComparatorBenchmarks.put                      java_reverse_bytewise_non-direct_no-reuse  thrpt   25    7942.286 ±  626.170  ops/s
ComparatorBenchmarks.put                          java_reverse_bytewise_direct_no-reuse  thrpt   25   11781.955 ± 1019.843  ops/s
```
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6252

Differential Revision: D19331064

Pulled By: pdillinger

fbshipit-source-id: 1f3b794e6a14162b2c3ffb943e8c0e64a0c03738
2020-02-03 12:30:13 -08:00
sdong
800d24ddc5 Fix DBTest2.ChangePrefixExtractor LITE build (#6356)
Summary:
DBTest2.ChangePrefixExtractor fails in LITE build because LITE build doesn't support adaptive build. Fix it by removing the stats check but only check correctness.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6356

Test Plan: Run the test with both of LITE and non-LITE build.

Differential Revision: D19669537

fbshipit-source-id: 6d7dd6c8a79f18e80ca1636864b9c71922030d8e
2020-01-31 15:44:14 -08:00
Maysam Yabandeh
01ab882ba3 Fix release warning for unused bg_canceled
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/6357

Differential Revision: D19670931

Pulled By: maysamyabandeh

fbshipit-source-id: d528c4c7f9450f1f38b9d2a36e0d5d0865b39be9
2020-01-31 15:09:10 -08:00
sdong
ec496347bc Add a unit test for prefix extractor changes (#6323)
Summary:
Add a unit test for prefix extractor change, including a check that fails due to a bug.
Also comment out the partitioned filter case which will fail the test too.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6323

Test Plan: Run the test and it passes (and fails if the SeekForPrev() part is uncommented)

Differential Revision: D19509744

fbshipit-source-id: 678202ca97b5503e9de73b54b90de9e5ba822b72
2020-01-31 11:02:03 -08:00