Summary:
To support column families, it is easiest to use VersionSet to manage
our column families (if we don't have Versions then ColumnFamilyData always
behaves as a dummy column family). This diff only refactors the existing repair
logic to use VersionSet; the next two parts will add support for multiple
column families.
Test Plan:
$ ./repair_test
Reviewers: yhchiang, IslamAbdelRahman, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59775
Summary: Fix two minor typos and update the file name which is used to trigger the runs in case new changes have been committed.
Test Plan: - Testing with a private Sandcastle instance.
Reviewers: sdong, mung
Reviewed By: mung
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59919
Summary:
Add a read option `background_purge_on_iterator_cleanup` to avoid deleting files in foreground when destroying iterators.
Instead, a job is scheduled in high priority queue and would be executed in a separate background thread.
Test Plan: Add a variant of PurgeObsoleteFileTest. Turn on background purge option in the new test, and use sleeping task to ensure files are deleted in background.
Reviewers: IslamAbdelRahman, sdong
Reviewed By: IslamAbdelRahman
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59499
Summary:
DB::AddFile() right now always add the ingested file to L0
update the logic to add the file to the lowest possible level
Test Plan: unit tests
Reviewers: jkedgar, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, yoshinorim
Differential Revision: https://reviews.facebook.net/D59637
Summary: filter_deletes option was removed, remove it from crash_test to fix it
Test Plan: make crash_test
Reviewers: yhchiang, andrewkr, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59901
Summary:
The main PHP code churn is caused by extracting the common code from `FacebookArcanistConfiguration.php` and `FacebookOldArcanistConfiguration.php` into `RocksDBCommonDeterminator.php`. This is necessary both for reducing the duplication of code and making sure that we can execute the common core logic separately from continuous runs.
The main logic in `RocksDBCommonDeterminator.php` remains quite the same with the exception of some things:
- Adding separation between the cases when a diff is submitted //vs.// when the code is triggered from a continuous run. There are certain actions which we should do in a case of diff only.
- Adding reporting - now the person who authored the diff will receive e-mail notifications if any of the jobs have failed.
- Enabling assertions and making sure that we'll terminate on failure. This is an internal code used by competent engineers, so instead of `if (!condition) { echo "Something"; exit(1); }` for every invariant I think that `assert(condition)` provides better readability with the same behavior. Especially taking into account that we're talking about things which shouldn't ever happen.
Enabling this entire process will be triggered internally and will be a subject of a separate code review. We should discuss the details of triggering continuous RocksDB build and tests on that diff.
Test Plan:
- Make sure that `[p]arc diff` scenario isn't broken by verifying that tests validating this diff will pass.
- Private testing of triggering the continuous build script.
- Once the changes will land then author an internal job which will use the script and verify its validity.
Reviewers: sdong, yhchiang, kradhakrishnan
Reviewed By: kradhakrishnan
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59811
Summary:
Update Makefile to show warnings when we have invalid paths in our make_config.mk file
sample output
```
$ make static_lib -j64
Makefile:150: Warning: /mnt/gvfs/third-party2/libgcc/53e0eac8911888a105aa98b9a35fe61cf1d8b278/4.9.x/gcc-4.9-glibc-2.20/024dbc3/libs dont exist
Makefile:150: Warning: /mnt/gvfs/third-party2/llvm-fb/b91de48a4974ec839946d824402b098d43454cef/stable/centos6-native/7aaccbe/../../src/clang/tools/scan-build/scan-build dont exist
GEN util/build_version.cc
```
Test Plan: check that warning is printed visually
Reviewers: sdong, yiwu, andrewkr
Reviewed By: andrewkr
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59523
Summary: SstFileWriter ignore Options::bottommost_compression, update it to use bottommost_compression if available
Test Plan:
make check -j64
verified used compression using ./sst_dump
Reviewers: sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, yoshinorim
Differential Revision: https://reviews.facebook.net/D59841
Summary: filter_deltes is not a frequently used feature. Remove it.
Test Plan: Run all test suites.
Reviewers: igor, yhchiang, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59427
Summary: We introduced default slow down and stop condition, but didn't reset it in bulk load mode. Fix it.
Test Plan: N/A
Reviewers: igor, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59757
Summary: Fixed a crash bug that incorrectly parse deprecated options in options_helper
Test Plan:
run db_bench with an old options file with memtable_prefix_bloom_probes
./db_bench --options_file=AN_OLD_OPTIONS_FILE --num=100 --benchmarks=fillseq
Reviewers: sdong, IslamAbdelRahman
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59787
Summary:
Allow arcanist_util to work with both new and old arc versions.
The diff is based on Adam Retter's pull request
https://github.com/facebook/rocksdb/pull/1168
Many thanks to Adam to initiate this work
Test Plan: run arc lint and arc diff using different arc versions
Reviewers: sdong, IslamAbdelRahman, kradhakrishnan, andrewkr, adamretter, igor
Reviewed By: adamretter
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59697
Summary:
We dont report the bytes that we ingested from AddFile which make the write amplification numbers incorrect
Update InternalStats and add logging for AddFile()
Test Plan: Make sure the code compile and existing tests pass
Reviewers: lightmark, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59763
Summary: DBCompactionTest.SkipStatsUpdateTest sometimes fails. I don't see any verification related to the deletes issued. Remove them to avoid the uncertainty.
Test Plan: Run the test.
Reviewers: IslamAbdelRahman, andrewkr, yhchiang
Reviewed By: yhchiang
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59613
Summary: char is not signed in some platforms. Having negative values confuse those compilers.
Test Plan: Run all existing tests.
Reviewers: andrewkr, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59619
Summary:
Right now the way we do BlockIter::Prev() is like this
- Go to the beginning of the restart interval
- Keep moving forward (and decoding keys using ParseNextKey()) until we reach the desired key
This can be optimized by caching the decoded entries in the first pass and reusing them in consecutive BlockIter::Prev() calls
Before caching
```
DEBUG_LEVEL=0 make db_bench -j64 && ./db_bench --benchmarks="readreverse" --db="/dev/shm/bench_prev_opt/" --use_existing_db --disable_auto_compactions
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.413 micros/op 2423972 ops/sec; 268.2 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.414 micros/op 2413867 ops/sec; 267.0 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.410 micros/op 2440881 ops/sec; 270.0 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.414 micros/op 2417298 ops/sec; 267.4 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.413 micros/op 2421682 ops/sec; 267.9 MB/s
```
After caching
```
DEBUG_LEVEL=0 make db_bench -j64 && ./db_bench --benchmarks="readreverse" --db="/dev/shm/bench_prev_opt/" --use_existing_db --disable_auto_compactions
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.324 micros/op 3088955 ops/sec; 341.7 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.335 micros/op 2980999 ops/sec; 329.8 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.341 micros/op 2929681 ops/sec; 324.1 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.344 micros/op 2908490 ops/sec; 321.8 MB/s
DB path: [/dev/shm/bench_prev_opt/]
readreverse : 0.338 micros/op 2958404 ops/sec; 327.3 MB/s
```
Test Plan: COMPILE_WITH_ASAN=1 make check -j64
Reviewers: andrewkr, yiwu, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, yoshinorim
Differential Revision: https://reviews.facebook.net/D59463
Summary: compile and link with jemalloc in examples binaries
Test Plan:
cd examples
make -j64
Reviewers: andrewkr, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, yoshinorim
Differential Revision: https://reviews.facebook.net/D59457
Summary:
Two changes here:
- Remove dead Jenkins related code which is no longer relevant.
- Support `arc diff --preview`. Currently it doesn't work because a step which applies a diff assumes that a revision has been created. Which in case of `--preview` isn't. Therefore diff can't be applied and validation fails. Solution is to use `--nocommit` because for validation purposes performing a commit isn't necessary.
Test Plan:
- Current changes are submitted using `arc diff --preview`.
- All the pre-commit verification tests passed.
Reviewers: kradhakrishnan, sdong
Reviewed By: sdong
Subscribers: leveldb, andrewkr, jtolmer, dhruba
Differential Revision: https://reviews.facebook.net/D59571
Summary:
We have alot of code duplication whenever we call FullMerge we keep duplicating the instrumentation and statistics code
This is a simple diff to refactor the code to use TimedFullMerge instead of FullMerge
Test Plan: COMPILE_WITH_ASAN=1 make check -j64
Reviewers: andrewkr, yhchiang, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59577
Summary: allow_concurrent_memtable_write and enable_write_thread_adaptive_yield went through extensive stress test and benchmarks. Should be considered stable.
Test Plan: N/A
Reviewers: ngbronson, yhchiang, IslamAbdelRahman, dhruba
Reviewed By: dhruba
Subscribers: MarkCallaghan, leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59415
Summary:
The tsan error was because the random implementation we have is not
thread safe, using Random::GetTLSInstance
Test Plan: Run tests in Linux
Reviewers: sdong
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59559
Summary:
Add option to not flush memtable on open()
In case the option is enabled, don't delete existing log files by not updating log numbers to MANIFEST.
Will still flush if we need to (e.g. memtable full in the middle). In that case we also flush final memtable.
If wal_recovery_mode = kPointInTimeRecovery, do not halt immediately after encounter corruption. Instead, check if seq id of next log file is last_log_sequence + 1. In that case we continue recovery.
Test Plan: See unit test.
Reviewers: dhruba, horuff, sdong
Reviewed By: sdong
Subscribers: benj, yhchiang, andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D57813
Summary: current libgcc lib path is broken, update it and make sure the new one exists
Test Plan:
verify that the new path exists
make check -j64
Reviewers: yiwu, sdong, andrewkr
Reviewed By: andrewkr
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59535
Summary:
Try to decompress compressed blocks when a special flag is set.
assert and crash in debug builds if we can't decompress the just-compressed input.
Test Plan: Run unit-tests.
Reviewers: dhruba, andrewkr, sdong, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59145
Summary: Need to change several more places for the change to fix Java tests
Test Plan:
make jtest
under java, run "make db_bench"
Reviewers: yhchiang, andrewkr, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59541
Summary: It confuses some compilers to have slice.cc under multiple directories. Merge them.
Test Plan: Run existing tests
Reviewers: andrewkr, yhchiang, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59409
Summary: Currently, if users define both of full key bloom and prefix bloom in SST files. During Get(), if full key bloom shows the key may exist, we still go ahead and check prefix bloom. This is wasteful. If bloom filter for full keys exists, we should always ignore prefix bloom in Get().
Test Plan: Run existing tests
Reviewers: yhchiang, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D57825
Summary: delete_scheduler_test and db_sst_test share a same directory name, causing possible fails on both tests when running in parallel. Fixed by changing directory name.
Test Plan: Run the two tests in parallel: `parallel -u ./{} ::: delete_scheduler_test db_sst_test`
Reviewers: sdong, andrewkr
Reviewed By: sdong, andrewkr
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59529
Summary: current clang_analyze path is broken, update it
Test Plan:
USE_CLANG=1 TEST_TMPDIR=/dev/shm/rocksdb OPT=-g make J=64 analyze -j64
make sure it works
Reviewers: yiwu, sdong, andrewkr
Reviewed By: andrewkr
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59505
Summary:
The sandcastle setup doesn't provide a remote with our branches. Need
to fetch them directly from github.
Test Plan:
ran script on devserver and the relevant commands on a sandcastle
host.
Reviewers: IslamAbdelRahman, lightmark, kradhakrishnan, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59511
Summary:
memtable_prefix_bloom_probes is not a critical option. Remove it to reduce number of options.
It's easier for users to make mistakes with memtable_prefix_bloom_bits, turn it to memtable_prefix_bloom_bits_ratio
Test Plan: Run all existing tests
Reviewers: yhchiang, igor, IslamAbdelRahman
Reviewed By: IslamAbdelRahman
Subscribers: gunnarku, yoshinorim, MarkCallaghan, leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59199
Summary:
Added new features to help run the entire suite of tests on a diff
locally leveraging parallelism
Test Plan: Basic command line test
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59445
Summary: Backup options file to private directory
Test Plan:
backupable_db_test.cc, BackupOptions
Modify DB options by calling OpenDB for 3 times. Check the latest options file is in the right place. Also check no redundent files are backuped.
Reviewers: andrewkr
Reviewed By: andrewkr
Subscribers: leveldb, dhruba, andrewkr
Differential Revision: https://reviews.facebook.net/D59373
Summary:
Added an option, --env_uri. When provided, it is used as an argument to
NewEnvFromUri(), which instantiates an Env based on it.
Test Plan:
built a simple binary that registers ChrootEnv for prefix "/", then
ran:
$ ./tmp --env_uri /tmp/ --db /abcde
/tmp/ is the chroot directory and /abcde is the db_name. Then I verified
db_bench uses /tmp/abcde
Reviewers: sdong, kradhakrishnan, lightmark
Reviewed By: lightmark
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59325
Summary: AFIK, options builder is not used by anyone. Remove it.
Test Plan: Run all existing tests.
Reviewers: IslamAbdelRahman, andrewkr, igor
Reviewed By: igor
Subscribers: leveldb, andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59319
Andrew Kryczka