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

options: Add the ability to generate and validate options per use case (#385) #471

Closed

Conversation

AmnonHanuhov
Copy link
Contributor

No description provided.

riversand963 and others added 30 commits September 18, 2022 21:45
Summary:
when there is a single memtable without range tombstones and no SST files in the database, DBIter should wrap memtable iterator directly. Currently we create a merging iterator on top of the memtable iterator, and have DBIter wrap around it. This causes iterator regression and this PR fixes this issue.

Pull Request resolved: facebook/rocksdb#10705

Test Plan:
- `make check`
- Performance:
  - Set up: `./db_bench -benchmarks=filluniquerandom -write_buffer_size=$((1 << 30)) -num=10000`
  - Benchmark: `./db_bench -benchmarks=seekrandom -use_existing_db=true -avoid_flush_during_recovery=true -write_buffer_size=$((1 << 30)) -num=10000 -threads=16 -duration=60 -seek_nexts=$seek_nexts`
```
seek_nexts    main op/sec    facebook/rocksdb#10705      RocksDB v7.6
0             5746568        5749033     5786180
30            2411690        3006466     2837699
1000          102556         128902      124667
```

Reviewed By: ajkr

Differential Revision: D39644221

Pulled By: cbi42

fbshipit-source-id: 8063ff611ba31b0e5670041da3927c8c54b2097d
Summary:
Fix potential memory leak in ArenaWrappedDBIter::Refresh() introduced in facebook/rocksdb#10705. See facebook/rocksdb#10705 (comment) for detail.

Pull Request resolved: facebook/rocksdb#10716

Test Plan: make check

Reviewed By: ajkr

Differential Revision: D39698561

Pulled By: cbi42

fbshipit-source-id: dc0d0c6e3878eaa84f87623fbe4916b9b08b077a
Summary:
Update io_uring_prep_cancel as it is now backward compatible.
Also, io_uring_prep_cancel expects sqe->addr to match with read
request submitted. It's being set wrong which is now fixed in this PR.

Pull Request resolved: facebook/rocksdb#10644

Test Plan:
- Ran internally with lastest liburing package and on RocksDB
github repo with older version.
- Ran seekrandom regression to confirm there is no regression.

Reviewed By: anand1976

Differential Revision: D39284229

Pulled By: akankshamahajan15

fbshipit-source-id: fd52cdf23d676da114896163626b75c8ae09c980
Summary:
Mention in HISTORY.md the fix in facebook/rocksdb#10705.

Pull Request resolved: facebook/rocksdb#10720

Reviewed By: riversand963

Differential Revision: D39709455

Pulled By: cbi42

fbshipit-source-id: 1a2c9dd8425c73f7095ddb1d0b1cca8ed35b7ef2
Summary:
Currently, without this fix, DBImpl::GetLatestSequenceForKey() may not return the latest sequence number for merge operands of the key. This can cause conflict checking during optimistic transaction commit phase to fail. Fix it by always returning the latest sequence number of the key, also considering range tombstones.

Pull Request resolved: facebook/rocksdb#10724

Test Plan: make check

Reviewed By: cbi42

Differential Revision: D39756847

Pulled By: riversand963

fbshipit-source-id: 0764c3dd4cb24960b37e18adccc6e7feed0e6876
Summary:
when a new internal iterator is constructed during iterator refresh, pointer to the previous memtable range tombstone iterator was not cleared. This could cause segfault for future `Refresh()` calls when they try to free the memtable range tombstones. This PR fixes this issue.

Pull Request resolved: facebook/rocksdb#10739

Test Plan: added a unit test in db_range_del_test.cc to reproduce this issue.

Reviewed By: ajkr, riversand963

Differential Revision: D39825283

Pulled By: cbi42

fbshipit-source-id: 3b59a2b73865aed39e28cdd5c1b57eed7991b94c
Summary:
Fix a bug in Iterator::Refresh() where the local SV it obtained could be obsolete upon return, and should be cleaned up.

Pull Request resolved: facebook/rocksdb#10770

Test Plan: added a unit test to reproduce the issue.

Reviewed By: ajkr

Differential Revision: D40063809

Pulled By: ajkr

fbshipit-source-id: 619e728eb0f1ac9540b4d0ad38e43acc37a514b2
…0767)

Summary:
fix for facebook/rocksdb#10752 where RocksDB could be in an infinite compaction loop (with compaction reason kBottommostFiles)  if allow_ingest_behind is enabled and the bottommost level is unfilled.

Pull Request resolved: facebook/rocksdb#10767

Test Plan: Added a unit test to reproduce the compaction loop.

Reviewed By: ajkr

Differential Revision: D40031861

Pulled By: ajkr

fbshipit-source-id: 71c4b02931fbe507a847632905404c9b8fa8c96b
…773)

Summary:
With current implementation, within the same RocksDB instance, all column families with non-empty memtables will be scheduled for flush if RocksDB determines that any column family needs to be flushed, e.g. memtable full, write buffer manager, etc., if atomic flush is enabled. Not doing so can lead to data loss and inconsistency when WAL is disabled, which is a common setting when atomic flush is enabled. Therefore, setting a per-column-family knob, min_write_buffer_number_to_merge to a value greater than 1 is not compatible with atomic flush, and should be sanitized during column family creation and db open.

Pull Request resolved: facebook/rocksdb#10773

Test Plan:
Reproduce: D39993203 has detailed steps.
Run the test with and without the fix.

Reviewed By: cbi42

Differential Revision: D40077955

Pulled By: cbi42

fbshipit-source-id: 451a9179eb531ac42eaccf40b451b9dec4085240
Summary:
We have seen some rare crash test failures in HyperClockCache, and the source could certainly be a bug fixed in this change, in ClockHandleTable::ConstApplyToEntriesRange. It wasn't properly accounting for the fact that incrementing the acquire counter could be ineffective, due to parallel updates. (When incrementing the acquire counter is ineffective, it is incorrect to then decrement it.)

This change includes some other minor clean-up in HyperClockCache, and adds stats_dump_period_sec with a much lower period to the crash test. This should be the primary caller of ApplyToEntries, in collecting cache entry stats.

Pull Request resolved: facebook/rocksdb#10768

Test Plan: haven't been able to reproduce the failure, but should be in a better state (bug fix and improved crash test)

Reviewed By: anand1976

Differential Revision: D40034747

Pulled By: anand1976

fbshipit-source-id: a06fcefe146e17ee35001984445cedcf3b63eb68
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.
…Data()

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.
maxb-io and others added 19 commits March 19, 2023 15:56
…db project. It is required for the build in order to reflect the environment on the runner machine (#442)
While at it, rename and reorder some members
Theres a sanitize check in case of allow_concurrent_memtable_write == 1 which
verifies that the memtablerep supports it. This commit prevents the code
from rerolling if the memtablerep was originally skip_list or
speedb.HashSpdRepFactory
There is a circular dependency between options.h and write_buffer_manager.h.
In order to fix that, I forward declare the WriteBufferManager class in
options.h and include it in options.cc and in other .cc files which
use WriteBufferManager.
use fixed num delay steps instead of fixing the step size and deducing
the num steps.
this makes the code simpler and avoids a bug when the excess bytes are
greater than num_steps * step_size which then fails the assert:
assert(step_num < num_steps);  - since step_num == num_steps.
EXPERIMENTAL feature.

This write flow doesn't wait for the WAL to finish a write before writing
concurrently to the memtable, shortening the amount of time threads have
to wait for a write to complete in a heavily multithreaded write scenario.
Currently even if objects have been expired we still return them until
compaction delete those objects.

This commit adds a skip_expired_data to the ReadOptions, if this option
is true, it Get, MultiGet and iterator will not return objects that have been
expired by the configured ttl.
Currently this feature does not include support for Merge operations.
For Read Only and iterator pinned data the skip_expired_data will be 
enforced and expired keys will not be returned.

Resolves: #394, #417

Pull Request: #403
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
#467)

* added temporary workflow to test artifact build with readline-dev installed

* commented out release action
* added temporary workflow to test artifact build with readline-dev installed

* commented out release action

* rename the workflow

* add readline-devel instead of readline-dev
* added temporary workflow to test artifact build with readline-dev installed

* commented out release action

* rename the workflow

* add readline-devel instead of readline-dev

* added readline-devel package to the artifact-release.yml, needed by beezcli
* added temporary workflow to test artifact build with readline-dev installed

* commented out release action

* rename the workflow

* add readline-devel instead of readline-dev

* added readline-devel package to the artifact-release.yml, needed by beezcli

* fixed version to 2.3, in was bumped up by a workflow that failed on building artifacts
…shtest

This class inherits from UseCase and is a singleton.
@AmnonHanuhov AmnonHanuhov requested a review from mrambacher April 8, 2023 11:12
@AmnonHanuhov AmnonHanuhov self-assigned this Apr 8, 2023
@AmnonHanuhov AmnonHanuhov linked an issue Apr 8, 2023 that may be closed by this pull request
@AmnonHanuhov AmnonHanuhov marked this pull request as draft April 8, 2023 11:12
@AmnonHanuhov
Copy link
Contributor Author

@mrambacher
A few things to take notice of:

  1. In C++ there isn't really a convenient way with enough abstractions to generate random numbers like there is in python.
  2. UseCase is an abstract class but should we declare GetInstance() there since this function will be implemented in each class that inherits from UseCase in order for it to be singleton?
  3. GetInstance() isn't thread-safe.
  4. The implementation of Validate is still not clear to me. The specific use case should know the valid range but where do we hold this range? Ideally I would want this range-per-option to appear in some sort of single point of truth so that db_crashtest.py, for example, can use it too. It can be a json or a yaml file where each option has the min and max value it can get and then we parse it in the python script and in the c++ class. Just a thought, let me know what you think.

@mrambacher
Copy link
Contributor

@mrambacher A few things to take notice of:

  1. In C++ there isn't really a convenient way with enough abstractions to generate random numbers like there is in python.

RocksDB has a Random number generator class in util/Random. If this is not sufficient for these purposes, that class should probably be improved.

  1. UseCase is an abstract class but should we declare GetInstance() there since this function will be implemented in each class that inherits from UseCase in order for it to be singleton?

There is no need for a GetInstance in this manner. Something similar to what is done for in util/Comparator should suffice.

  1. GetInstance() isn't thread-safe.

One option here might be std::once

  1. The implementation of Validate is still not clear to me. The specific use case should know the valid range but where do we hold this range? Ideally I would want this range-per-option to appear in some sort of single point of truth so that db_crashtest.py, for example, can use it too. It can be a json or a yaml file where each option has the min and max value it can get and then we parse it in the python script and in the c++ class. Just a thought, let me know what you think.

Are the parameters validated in the crash test scripts? I think you can just code the ranges into the C++ class and do away with the code in the crash test scripts but am not positive.

@AmnonHanuhov
Copy link
Contributor Author

AmnonHanuhov commented Apr 12, 2023

@mrambacher A few things to take notice of:

  1. In C++ there isn't really a convenient way with enough abstractions to generate random numbers like there is in python.

RocksDB has a Random number generator class in util/Random. If this is not sufficient for these purposes, that class should probably be improved.

It isn't sufficient, for example it doesn't support choosing a random number from a given set of numbers rather than a range. I should probably start from adding this feature.

  1. UseCase is an abstract class but should we declare GetInstance() there since this function will be implemented in each class that inherits from UseCase in order for it to be singleton?

There is no need for a GetInstance in this manner. Something similar to what is done for in util/Comparator should suffice.

In Comparator class I see that there is an assignment operator implemented, so it isn't a singleton, to my understanding.

  1. The implementation of Validate is still not clear to me. The specific use case should know the valid range but where do we hold this range? Ideally I would want this range-per-option to appear in some sort of single point of truth so that db_crashtest.py, for example, can use it too. It can be a json or a yaml file where each option has the min and max value it can get and then we parse it in the python script and in the c++ class. Just a thought, let me know what you think.

Are the parameters validated in the crash test scripts? I think you can just code the ranges into the C++ class and do away with the code in the crash test scripts but am not positive.

Doesn't seem like the script validates the parameters. What if someone decides to change the range or the set of numbers from which we sample, for a given option in the script, he will then have to remember to change it in the c++ code as well. I feel like implementing it that way is kind of error-prone.

@mrambacher
Copy link
Contributor

@mrambacher A few things to take notice of:

  1. In C++ there isn't really a convenient way with enough abstractions to generate random numbers like there is in python.

RocksDB has a Random number generator class in util/Random. If this is not sufficient for these purposes, that class should probably be improved.

It isn't sufficient, for example it doesn't support choosing a random number from a given set of numbers rather than a range. I should probably start from adding this feature.

Can you give me an example of a range that you are talking about? There is already a "Random::Uniform" method. Is that not sufficient?

BTW, there is also a "Random64" class, which seems very similar to what you were proposing.

  1. UseCase is an abstract class but should we declare GetInstance() there since this function will be implemented in each class that inherits from UseCase in order for it to be singleton?

There is no need for a GetInstance in this manner. Something similar to what is done for in util/Comparator should suffice.

In Comparator class I see that there is an assignment operator implemented, so it isn't a singleton, to my understanding.

A comparator is an abstract class. Most likely, those assignments are for an InternalComparator and not a Comparator per-se.

  1. The implementation of Validate is still not clear to me. The specific use case should know the valid range but where do we hold this range? Ideally I would want this range-per-option to appear in some sort of single point of truth so that db_crashtest.py, for example, can use it too. It can be a json or a yaml file where each option has the min and max value it can get and then we parse it in the python script and in the c++ class. Just a thought, let me know what you think.

Are the parameters validated in the crash test scripts? I think you can just code the ranges into the C++ class and do away with the code in the crash test scripts but am not positive.

Doesn't seem like the script validates the parameters. What if someone decides to change the range or the set of numbers from which we sample, for a given option in the script, he will then have to remember to change it in the c++ code as well. I feel like implementing it that way is kind of error-prone.

I think the point is that the script would not generate these random parameters any longer. Instead, the script would say generate a "crash usage" set of options and that would be what is used. If "option validation" validation is enabled, the DB::Open (or something else) would validate that the options are in the range which are acceptable to the installed use cases, such as "crash usage". So crash usage would potentially have two places (in validation and generation) where it knows what the range of acceptable parameters for it are.

@AmnonHanuhov AmnonHanuhov deleted the 385-configuration-validation-while-opening-a-db branch May 12, 2023 15:06
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.

Configuration validation while opening a DB