Summary:
Currently, RocksDB lets compaction to go through even in case of
corrupted keys, the number of which is reported in CompactionJobStats.
However, RocksDB does not check this value. We should let compaction run
in a stricter mode.
Temporarily disable two tests that allow corrupted keys in compaction.
With this PR, the two tests will assert(false) and terminate. Still need
to investigate what is the recommended google-test way of doing it.
Death test (EXPECT_DEATH) in gtest has warnings now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7124
Test Plan: make check
Reviewed By: ajkr
Differential Revision: D22530722
Pulled By: riversand963
fbshipit-source-id: 6a5a6a992028c6d4f92cb74693c92db462ae4ad6
Summary:
https://github.com/facebook/rocksdb/pull/6850, which added compaction
filter support to BlobDB, reused elements of the BlobDB GC mechanism.
This patch updates some log messages in this logic to account for this
fact; namely, it replaces mentions of "GC" with "compaction/GC" to avoid
confusion in cases when GC is not enabled.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7128
Test Plan: `make check`
Reviewed By: zhichao-cao
Differential Revision: D22535371
Pulled By: ltamasi
fbshipit-source-id: 1f14f3b02ab9983728bbca1cf680420208d9a195
Summary:
This fixes an issue introduced in 0c56fc4 whereby the location of Python is evaluated many times and leads to excessive logging of unknown python locations of CentOS 6.
The location is now only checked once.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7123
Reviewed By: zhichao-cao
Differential Revision: D22532274
Pulled By: ajkr
fbshipit-source-id: cade71b4b46e9a23d63ecb4dd36a4ac8ae217970
Summary:
It is helpful to add some TSAN coverage before a pull request is committed. This diff adds some of them.
Some slow tests are excluded for the running speed. Some are blacklisted because they show warnings. Will investigate these warnings and see whether we can fix or suppress them.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7122
Test Plan: Watch CIrcleCI runs
Reviewed By: riversand963
Differential Revision: D22532133
fbshipit-source-id: 81ddd02d9df19c513a12811979e8ddabae911354
Summary:
Periodic syncing of blob files is performed by `WritableFileWriter`;
`bytes_per_sync_` and `next_sync_offset_` in `BlobLogWriter` are
actually unused (or more precisely, only used by methods that are
themselves unused). The patch removes all this dead code.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7125
Test Plan: `make check`
Reviewed By: riversand963
Differential Revision: D22531021
Pulled By: ltamasi
fbshipit-source-id: 6b293ad5a79d3e6bf15c5c68f7aedd7ce7a15f10
Summary:
During memtable lookup, an unrecognized value type should be reported as
Status::Corruption.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7121
Test Plan: make check
Reviewed By: cheng-chang
Differential Revision: D22512124
Pulled By: riversand963
fbshipit-source-id: 9b97be7d9b230c5aae9205f96054420e5ea09066
Summary:
1. Add the wrapper classes FileSystemTracingWrapper, FSSequentialFileTracingWrapper, FSRandomAccessFileTracingWrapper, FSWritableFileTracingWrapper, FSRandomRWFileTracingWrapper that forward the calls to underlying storage system and then pass the file operation information to IOTracer. IOTracer dumps the record in binary format for tracing.
2. Add the wrapper classes FileSystemPtr, FSSequentialFilePtr, FSRandomAccessFilePtr, FSWritableFilePtr and FSRandomRWFilePtr that overload operator-> and return ptr to underlying storage system or Tracing wrapper class based on enabling/disabling of IO tracing. These classes are added to bypass Tracing Wrapper classes when we disable tracing.
3. Add enums in trace.h that distinguish which options need to be added for different file operations(Read, close, write etc) as part of tracing record.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7002
Test Plan: make check -j64
Reviewed By: anand1976
Differential Revision: D22127897
Pulled By: akankshamahajan15
fbshipit-source-id: 74cff58ce5661c9a3832dfaa52483f3b2d8565e0
Summary:
CLANG analyze is useful before pull request. Add it.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7114
Test Plan: Watch the CI results to succeed.
Reviewed By: riversand963
Differential Revision: D22491942
fbshipit-source-id: 9ccad91c6142fedc3d3dd491cf55054827908f36
Summary:
It is useful to run UBSAN before merging a PR. This commit adds it. We see warning for stl_tree.h, suppress the warning to make it work.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7112
Test Plan: See the CI to succeed. Manually ingest a UBSAN warning and see it got reported.
Reviewed By: pdillinger
Differential Revision: D22490519
fbshipit-source-id: e4495a0c78a3e2dae7dbf294da79585e141cbb66
Summary:
There currently exist multiple `GetChildren()` calls in `DBImpl::Recover()`, which can be expensive in cases of distributed file systems.
This pull request try to call `DBImpl::Recover()` of each necessary directory only _once_ and reuse the results in the places of repeated calls in current code.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7044
Test Plan:
Run `make check` and use the default test suite. The modified code should be semantically identical to the current code. As a proof of this solution, we may optionally deploy the system onto a (real or simulated) distributed system and expect reduced latency caused by manifest fetching.
(WIP)
Reviewed By: riversand963
Differential Revision: D22419925
Pulled By: roghnin
fbshipit-source-id: d3774fbfbc246c5527101bc16747eb5c90919886
Summary:
In status.cc, the assert is `assert(sizeof(msgs) > index)`; msgs is a const char* array, sizeof(msgs) is the array size*char* size, which will make the assert pass all the time. Change it to sizeof(msgs)/sizeof(char*) > index.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7045
Test Plan: pass make check
Reviewed By: cheng-chang
Differential Revision: D22291337
Pulled By: zhichao-cao
fbshipit-source-id: 4ba8ebbb8da80ace7ca6adcdb0c66726f993659d
Summary:
Cleans up some of the dependencies on test code in the Makefile while building tools:
- Moves the test::RandomString, DBBaseTest::RandomString into Random
- Moves the test::RandomHumanReadableString into Random
- Moves the DestroyDir method into file_utils
- Moves the SetupSyncPointsToMockDirectIO into sync_point.
- Moves the FaultInjection Env and FS classes under env
These changes allow all of the tools to build without dependencies on test_util, thereby simplifying the build dependencies. By moving the FaultInjection code, the dependency in db_stress on different libraries for debug vs release was eliminated.
Tested both release and debug builds via Make and CMake for both static and shared libraries.
More work remains to clean up how the tools are built and remove some unnecessary dependencies. There is also more work that should be done to get the Makefile and CMake to align in their builds -- what is in the libraries and the sizes of the executables are different.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7097
Reviewed By: riversand963
Differential Revision: D22463160
Pulled By: pdillinger
fbshipit-source-id: e19462b53324ab3f0b7c72459dbc73165cc382b2
Summary:
This is a followup to https://github.com/facebook/rocksdb/issues/6646. In that PR, for simplicity I just appended a comparison against the 0th restart key in case `BinarySeek()`'s binary search landed at index 0. As a result there were `2/(N+1) + log_2(N)` key comparisons. This PR does it differently. Now we expand the binary search range by one so it also covers the case where target is at or before the restart key at index 0. As a result, it involves `log_2(N+1)` key comparisons.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7068
Test Plan:
ran readrandom with mostly default settings and counted key comparisons
using `PerfContext`.
before: `user_key_comparison_count = 28881965`
after: `user_key_comparison_count = 27823245`
setup command:
```
$ TEST_TMPDIR=/dev/shm/dbbench ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=1048576 -target_file_size_base=1048576 -max_bytes_for_level_base=4194304 -max_background_jobs=12 -level_compaction_dynamic_level_bytes=true -num=10000000
```
benchmark command:
```
$ TEST_TMPDIR=/dev/shm/dbbench/ ./db_bench -use_existing_db=true -benchmarks=readrandom -disable_auto_compactions=true -num=10000000 -compression_type=none -reads=1000000 -perf_level=3
```
Reviewed By: anand1976
Differential Revision: D22357032
Pulled By: ajkr
fbshipit-source-id: 8b01e9c1c2a4e9d02fc9dfe16c1cc0327f8bdf24
Summary:
As title. The goal is to shorten the execution time of several tests
when they are combined together in a single TEST_F.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7105
Test Plan:
make db_wal_test
./db_wal_test
Reviewed By: ltamasi
Differential Revision: D22442705
Pulled By: riversand963
fbshipit-source-id: 0ad49b8f21fa86dcd5a4d3c9a06af313735ac217
Summary:
Although PR https://github.com/facebook/rocksdb/issues/7032 fixes the construction of the `SstFileDumper` in `GetFileDbIdentities` by setting a proper `Env` of the `Options` passed in the constructor, the file path was not corrected accordingly. This actually disables backup engine to use db session ids in the file names since the `db_session_id` is always empty.
Now it is fixed by setting the correct path in the construction of `SstFileDumper`. Furthermore, to preserve the Direct IO property that backup engine already has, parameter `EnvOptions` is added to `GetFileDbIdentities` and `SstFileDumper`.
The `BackupUsingDirectIO` test is updated accordingly.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7104
Test Plan: backupable_db_test and some manual tests.
Reviewed By: ajkr
Differential Revision: D22443245
Pulled By: gg814
fbshipit-source-id: 056a9bb8b82947c5e73d7c3fbb62bfe23af5e562
Summary:
When format_version is high enough to support user-key and
there are index entries for same user key that spans multiple data
blocks then it changes from user-key mode to internal-key mode. But the
flush policy is not reset to point to Block Builder of internal-keys.
After this switch, no entries are added to user key index partition
result, thus it never triggers flushing the block.
Fix: 1. After adding the entry in sub_builder_index_, if there is a switch
from user-key to internal-key, then flush policy is updated to point to
Block Builder of internal-keys index partition.
2. Set sub_builder_index_->seperator_is_key_plus_seq_ = true if
seperator_is_key_plus_seq_ is set to true so that subsequent partitions
can also use internal key mode.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7096
Test Plan: make check -j64
Reviewed By: ajkr
Differential Revision: D22416598
Pulled By: akankshamahajan15
fbshipit-source-id: 01fc2dc07ea1b32f8fb803995ebe6e9a3fbe67ac
Summary:
Delicious copy-pasta from https://github.com/facebook/rocksdb/issues/7039
Also fixing DestroyDir to allow files to go missing while it is operating. This seems to fix failures I got with test plan reproducer.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7103
Test Plan:
make blackbox_crash_test_with_atomic_flush for a while with
checkpoint_one_in=100
Reviewed By: siying
Differential Revision: D22435315
Pulled By: pdillinger
fbshipit-source-id: 0ec0538402493887aeda43ecc03f32979cb84ced
Summary:
The fix in PR https://github.com/facebook/rocksdb/issues/7082 is not really successful because there is still a small chance that the test will fail.
In addtion to flushing, we close the DB and then reopen before corrupting a table file in the DB. Specifically, we corrupt a table file before backup takes place as follows.
* Open DB
* Fill DB
* Flush DB (optional, no flushing here also works)
* Close DB
* Reopen DB
* Corrupt a table file in the DB
This should make the test reliable.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7102
Test Plan:
`while ./backupable_db_test --gtest_filter=*TableFileCorruptedBeforeBackup*; do true; done`
(kept running for an hour or so :)
Reviewed By: pdillinger
Differential Revision: D22432417
Pulled By: gg814
fbshipit-source-id: d407eee93ff428bb662f80cde1659fbf0149d0cd
Summary:
Currently, `EventListener` in listner.h only have callback functions for file read and write. One may favor extended callback functions for more file I/O operations like flush, sync and close. This PR tries to add those interface and have them called when appropriate throughout the code base.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7055
Test Plan:
Write an experimental listener with those new callback functions with log output in them; run experiments and check logs to see those functions are actually called.
Default test suits `make check` should also be included.
Reviewed By: riversand963
Differential Revision: D22380624
Pulled By: roghnin
fbshipit-source-id: 4121491d45c2c2aae8c255e7998090559a241c6a
Summary:
On some platforms like MacOS, a second 'make check' can lead to
/bin/rm: Argument list too long
This is fixed by replacing with a 'find'. Also, using '-f' for more rm calls
to avoid prompt.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7095
Test Plan: 'make check' on Linux and MacOS
Reviewed By: riversand963
Differential Revision: D22415808
Pulled By: pdillinger
fbshipit-source-id: 0fd1ebae13739c9d81f9e813e99b062715604d6b
Summary:
by tracking and linking against runtime dependent libraries in
Makefile
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7098
Test Plan: look for fix in CircleCI
Reviewed By: riversand963
Differential Revision: D22420860
Pulled By: pdillinger
fbshipit-source-id: d211d709214bf5306db68e43b7a2f18169281022
Summary:
Primarily, this change adds a way to work around a bug limiting the effective output (and therefore debugability) of the Linux builds using parallel make. We would get
make[1]: write error: stdout
probably due to a kernel bug, apparently affecting both available ubuntu 16 machine images (maybe not affecting docker images, less horsepower). https://bugs.launchpad.net/ubuntu/+source/linux-signed/+bug/1814393
Now in the CircleCI config, make output on Ubuntu is piped through a custom 'cat' that ignores EAGAIN errors, which seems to fix the problem.
Significant other changes:
* Add another linux build that combines
* LIB_MODE=shared, to ensure this works with compile and unit test execution
* Alternative rocksdb namespace, to ensure this works (not rely on Travis)
* ASSERT_STATUS_CHECKED=1, but with building all unit tests and running those expected to pass with it
* Run release build with and without gflags. (Was running only without, ignore large swaths of code in a normal release build! Two regressions in this build, only with gflags, in the last week not caught by CI!)
* Use gflags with unity and LITE build, as typical case.
Debugability improvements:
* Use V=1 to show commands being executed (thanks to EAGAIN work-around)
* Print kernel version and compiler versions as part of V=1 output from Makefile
Cosmetic other changes:
* Put more commands on one line, for less clutter in CircleCI output pages
* Remove redundant "all" in "make all check" and put make command options before targets
* Change some recursive "make clean" into dependency on "clean," toward minimizing unnecessary overhead (detect platform, build version, etc.) of extra recursive makes
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7078
Reviewed By: siying
Differential Revision: D22391647
Pulled By: pdillinger
fbshipit-source-id: d446fccf5a8c568b37dc8748621c8a5c546fe135
Summary:
(a) use STRESS_LIBRARY for db_stress and make sure
STRESS_LIBRARY has other stress test dependencies (as in buck build)
(b) fix rpath option to be accepted on MacOS. It still doesn't fully work
for me e.g. to run a LIB_MODE=shared unit test binary from another
directory, as it does on Linux, but the option is now accepted, and running
unit tests from current directory works for me.
Also adding LIB_MODE=shared to Travis. (Later TBD where best to fit in
in CircleCI.)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7066
Test Plan: manual
Reviewed By: cheng-chang
Differential Revision: D22364068
Pulled By: pdillinger
fbshipit-source-id: 6fa98a222f89f808ee786474de1100d92c1adec3
Summary:
If the corruption of a table file is done before flushing, then db manifest may record the checksum for the corrupted table, which results in "matching checksums" when backup engine tries to verfiy the checksum, and causes a flaky test.
Fix the issue by adding `Flush()` before trying to corrupt a table file in *db*.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7082
Test Plan:
`buck test`
Without the fix, failed 5 of 100 tests.
Suspected whether the pseudo randomness causes the issue: doubling `keys_iteration` resulted in 2 of 100 tests failed; deterministically corrupting tables file also caused 2 of 100 tests to fail.
With the fix, passed 200 of 200 tests.
Reviewed By: pdillinger
Differential Revision: D22375421
Pulled By: gg814
fbshipit-source-id: 7304618e7520684b6087e42d0b58329c5ad18329
Summary:
This is to fix special logic to run tests inside FB.
Buck test is broken after moving to cpp_unittest(). Move c_test back to the previous approach.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7076
Test Plan: Watch the Sandcastle run
Reviewed By: ajkr
Differential Revision: D22370096
fbshipit-source-id: 4a464d0903f2c76ae2de3a8ad373ffc9bedec64c
Summary:
When table file checksums are enabled and stored in the DB manifest by using the RocksDB default crc32c checksum function, BackupEngine will calculate the crc32c checksum of the file to be copied and compare the calculated result with the one stored in the DB manifest before copying the file to the backup directory.
After copying to the backup directory, BackupEngine will verify the checksum of the copied file with the one calculated before copying. This helps detect some rare corruption events such as bit-flips during the copying process.
No verification with checksums in DB manifest will be performed if the table file checksum function is not the RocksDB default crc32c checksum function.
In addition, If `share_table_files` and `share_files_with_checksum` are true, BackupEngine will compare the checksums computed before and after copying of the table files.
Corresponding tests are added.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7015
Test Plan: Passed make check
Reviewed By: pdillinger
Differential Revision: D22165732
Pulled By: gg814
fbshipit-source-id: ee0e8cc397c455eba64545c29380b9d9853588ec
Summary:
After https://github.com/facebook/rocksdb/pull/7036, we still see extra DBTest that can timeout when running 10 or 20 in parallel. Expand skip-fsync mode in whole DBTest. Still preserve other tests from doing this mode to be conservative.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7049
Test Plan: Run all existing files.
Reviewed By: pdillinger
Differential Revision: D22301700
fbshipit-source-id: f9a9e3b3b26ce640665a47cb8bff33ba0c89b565
Summary:
GetFileDbIdentities requires either db_id non-null or db_session_id non-null.
Passing nullptr for db_id or db_session_id in CopyOrCreateFile indicates the caller does not want to obtain the value for db_id or db_session_id.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7063
Test Plan:
USE_CLANG=1 make analyze
backupable_db_test
Reviewed By: pdillinger
Differential Revision: D22338497
Pulled By: gg814
fbshipit-source-id: 2aa2dcc14d156b0f99b07d6cf3c731ee088272cd
Summary:
When format_version is high enough to support user-key and there are index entries for same user key that spans multiple data blocks then it changes from user-key mode to internal-key mode. But the flush policy is not reset to point to Block Builder of internal-keys. After this switch, no entries are added to user key index partition result, thus it never triggers flushing the block.
Fix: After adding the entry in sub_builder_index_, if there is a switch from user-key to internal-key, then flush policy is updated to point to Block Builder of internal-keys index partition.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7022
Test Plan:
1. make check -j64
2. Added one unit test case
Reviewed By: ajkr
Differential Revision: D22197734
Pulled By: akankshamahajan15
fbshipit-source-id: d87e9e46bccab8e896ee6979d6b79c51f73d479e
Summary:
This check is flaky because compaction could run between the `Flush()` and the `TestGetTickerCount()`, which would increase the `BLOCK_CACHE_INDEX_MISS` count beyond what the test expects. Verified by adding a `sleep(1)` between those two lines and observing the counter is too high every time. The solution is just to remove this check as it doesn't have any use anyways. The latter check of index miss is sufficient to conclude the newest L0 file (i.e., the one generated by intra-L0) does not have its index block pinned in cache. It'd be nice to simultaneously check the L0 files generated by flush do have their index blocks pinned in cache, but that's not what the line deleted in this PR was checking..
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7065
Reviewed By: pdillinger
Differential Revision: D22340327
Pulled By: ajkr
fbshipit-source-id: e076b2c7228b7fa763dd0c0cb13828e176c1abee
Yanqin Jin