Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP (DNM): Add tests switching the FilterPolicy in the TableOptions on an existing tables #546

Open
wants to merge 230 commits into
base: main
Choose a base branch
from

Conversation

mrambacher
Copy link
Contributor

Added a few tests to start the discussion. Have not debugged them yet, but it appears like extra keys are thought to exist.

ltamasi and others added 30 commits March 18, 2023 10:49
Summary: Pull Request resolved: facebook/rocksdb#11319

Reviewed By: cbi42

Differential Revision: D44308743

Pulled By: ltamasi

fbshipit-source-id: ffd054e9f4162797cfe1ef78240ad2501f78bbbd
Summary:
**Context/Summary:**
As title.

Pull Request resolved: facebook/rocksdb#11355

Test Plan:
- Ran previously failed tests and they succeed
- Perf
`./db_bench -seed=1679014417652004 -db=/dev/shm/testdb/ -statistics=false -benchmarks="fillseq[-X60]" -key_size=32 -value_size=512 -num=100000 -db_write_buffer_size=655 -target_file_size_base=655 -disable_auto_compactions=false -compression_type=none -bloom_bits=3`

Reviewed By: ajkr

Differential Revision: D44719333

Pulled By: hx235

fbshipit-source-id: 23d22f314144071d97f7106ff1241c31c0bdf08b
Summary:
Platform009 is no longer supported in fbcode.

Pull Request resolved: facebook/rocksdb#11333

Reviewed By: pdillinger, ltamasi

Differential Revision: D44486431

Pulled By: anand1976

fbshipit-source-id: 99e19a70ebbb04ae750d39c33a110518bb25487e
Summary:
VerifyFileChecksums currently interprets the readahead_size as a payload of readahead_size for calculating the checksum, plus a prefetch of an additional readahead_size. Hence each read is readahead_size * 2. This change treats it as chunks of readahead_size for checksum calculation.

Pull Request resolved: facebook/rocksdb#11328

Test Plan: Add a unit test

Reviewed By: pdillinger

Differential Revision: D44718781

Pulled By: anand1976

fbshipit-source-id: 79bae1ebaa27de2a13bc86f5910bf09356936e63
RocksDB currently fails to build with clang 12 due to newly added
diagnostics. This is problematic for fuzz testing in CI and for developers
building locally with clang, and thankfully these are easily fixable.
Currently the CMake config only looks for ccache, so developers using
sccache need to manually set the launch commands. Add a check for sccache
to spare them the trouble.

While at it, clean up the CMake config and remove ccache as the link launch
wrapper since it will just forward to the underlying linker anyway, and
also regenerate the buck TARGETS file (we might want to drop support for
it at some point, but for now let's keep it consistent).
This commit originally imported fuzzing tools from rocksdb.
However, after the rebase on 6.21 (from 6.11.4) only the compilation
fixes are left.

Original commit message:

Building them is a bit of a pain, but the OSS-Fuzz build scripts are
a good reference on how to do that:
https://github.com/google/oss-fuzz/tree/master/projects/rocksdb

This commit also fixes some clang compilation errors.
…mUnsyncedData()

When there's no data in the buffer, there's nothing to drop anyway, and
providing 0 to the rand->Uniform() function results in a division by zero
error (SIGFPE), so just check and do nothing in that case.
When fixing the clang compilation errors (offsetof() being applied
to non standard layout types), the pointer flags were mistakenly set
to `kNone` from `kAllowNone`. This didn't cause an issue with the tests
on 6.22.1, but it does after the rebase on 7.2.2. Add the missing flags
back so that the test passes.
This helps debug things by correlating with other events in the system,
which we are unable to connect currently due to lack of information on
which process failed and when.
SPDB-225 added copying of the database during tests in order to preserve
the state of the database, but after a successful run there's no reason
to keep these copies around, so remove them.
The command is printed anyway by execute_cmd(), so there's no need to
print it in whitebox_crash_main() as well.
The current code is broken, because it effectively sets bool() as the
conversion function for boolean arguments, but bool() returns True for
every non-empty string, so that doesn't work.

Add a converter function to parse boolean value and set it as the type
for argparse in case the argument is of type bool.
…snapshot

In batched snapshot mode each key is duplicated 10 times and prefixed with
an ASCII digit in the range 0-9. However, the snapshot verification code
assumed that each key is present exactly as generated and exactly once,
so the calculated key index was bogus and lead to an invalid memory access.

Fix it by making the snapshot verification code aware of the batched mode
and apply the key conversion and verification accordingly.

While at it, clean up the way the verification bit vector is allocated.
in addition, add randomize_operation_type_percentages
This changes the way the stress test parameters are generated in order to
increase the coverage of the test and check for more edge cases.

Additionally, this change adds the option to run whitebox tests with kill
points disabled for testing functionality without crash recovery.

SPDB-390: crash_test: fix bool flags

SPDB-388: crash_test: disallow zero key dist val

zero vals in key_len_percent_dist are incompatible with
db_stress_common.h::GetIntVal
…ncy is on

running with both options, the stress test chosen is cf_consistency_stress
which does not use '0' - '9' as prefix for keys as in batched_ops_stress.
when doing compare_full_db_state_snapshot, the code expects each key to
have the prefix since test_batches_snapshots==true.
there is no point when both flags are on since each is run as a different stress test.
AmnonHanuhov and others added 22 commits May 11, 2023 10:11
…db-io#495)

* gitignore: Add build_tools/pycache/ to gitignore (speedb-io#486)

*.pyc artifacts from running gtest-parallel as part of `make check`
are created in this directory

* Makefile: Remove pycache artifacts after running gtest-parallel (speedb-io#486)
fix - AXV512: don't use XXH3_accumulate_512 with ASAN (speedb-io#398)
the previous commit disabled all the rest of the XXH optimizations
since it didnt prevent the XXH_VECTOR from being defined as XXH_AVX512.
when verify_before_write = true, msg_prefix was misplaced and the value from the db was inserted instead of it which led to junk and a decoding error.
Updated the history file with all of the content until 2.4.1 release.
* Set initiate_wbm_flushes to the same default value used by the WBM (true)

* Remove sanitizations of wbm flags in the db_bench, db_stress, and db_crashtest.py

* Create a WBM unconditionally in db_bench / db_stress (using the applicable flags)
Also switch to waiting a sec on the CV each time. This is required
since a bg error doesn't signal the CV in the WriteController.
Allow slowing down writes based on memory consumption by having the WBM raise a delay requirement in the WriteController.
The delay requirement is stored in the WC as part of the Global Delay feature (speedb-io#346). The same way a CF signals the WC that it has a delay requirement.
@mrambacher mrambacher requested a review from udi-speedb June 8, 2023 16:56
@udi-speedb
Copy link
Contributor

@mrambacher - It's a wip so this is not a full review.
It certainly seems to show that your observations are correct and it's not working properly.

@udi-speedb
Copy link
Contributor

@mrambacher - One more thing.
What about tests that start with no filter policy (create the table without a filter) and reopen the DB with an existing filter policy?
And the reverse - Creating the table with t a filter policy and reopening the DB without.

@udi-speedb
Copy link
Contributor

@mrambacher - Please open applicable issues that describe the associated issues that this PR exposes. Thanks

@udi-speedb
Copy link
Contributor

@mrambacher - Could you please open the issues?

@CLAassistant
Copy link

CLAassistant commented Feb 1, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 7 committers have signed the CLA.

✅ Yuval-Ariel
✅ udi-speedb
✅ maxb-io
✅ ofriedma
❌ bosmatt
❌ AmnonHanuhov
❌ mrambacher
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should be able to use different FilterPolicy for different SST files