Validate subrange reads in simulation by sfc-gh-anoyes · Pull Request #7597 · apple/foundationdb

sfc-gh-anoyes · 2022-07-15T00:27:12Z

We need test coverage of reading subranges within special key space modules, as
there was a bug that could only manifest if you read a subrange of the
conflicting keys module.

Add a utility that provides validation to special key space subrange reads in simulation
Fix bugs turned up by validating subrange reads

Code-Reviewer Section

The general guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

The PR has a description, explaining both the problem and the solution.
The description mentions which forms of testing were done and the testing seems reasonable.
Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

fdb-windows-ci · 2022-07-15T00:45:27Z

Doxense CI Report for Windows 10

Commit ID: 0331b35
Result: ❌ FAILED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-15T01:26:21Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 0331b35
Duration 0:59:02
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

sfc-gh-anoyes · 2022-07-15T01:27:07Z

I'm going to mark this is a draft for now since there are still more modules that should probably derive from SpecialKeyRangeAsyncImpl

foundationdb-ci · 2022-07-15T04:45:11Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 0331b35
Duration 4:17:47
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

sfc-gh-anoyes · 2022-07-18T16:07:00Z

I think I want to rework this so that it's a general test utility where you supply range read arguments, a range result, and a transaction and it will validate that you get the expected result if you re-read a subrange - assuming that reads always give the same results. This doesn't quite accomplish the goal of automatically providing test coverage for new modules, but I think that even in the transactional modules we can't assume we get the same result if we re-read since other things may be accessing that transaction concurrently, and thus changing the expected result. What do you think @sfc-gh-ajbeamon @sfc-gh-clin ?

It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently.

fdb-windows-ci · 2022-07-20T21:31:25Z

Doxense CI Report for Windows 10

Commit ID: 4720b4c
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-20T21:47:20Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 4720b4c
Duration 1:02:32
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

fdb-windows-ci · 2022-07-20T21:48:53Z

Doxense CI Report for Windows 10

Commit ID: 3c4a437
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-20T22:17:17Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: 3c4a437
Duration 0:43:23
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-20T22:37:42Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 3c4a437
Duration 1:04:35
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T01:01:39Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 4720b4c
Duration 4:16:49
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T01:49:50Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 3c4a437
Duration 4:16:44
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

fdb-windows-ci · 2022-07-21T19:09:23Z

Doxense CI Report for Windows 10

Commit ID: de272ab
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T19:33:26Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: de272ab
Duration 0:42:26
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T20:02:44Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: de272ab
Duration 1:11:46
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T21:44:27Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: de272ab
Duration 2:53:30
Result: ❌ FAILED
Error: Error while executing command: make -C tests -kj run. Reason: exit status 2
Build Logs (available for 30 days)

* fix heap-use-after-free caused by early noErrorsActors destroy * add comment * move keys out of StorageMetrics * format code * Java bindings for BG Purge (#6816) * Add support for returning whether a key-backed type range read has more results or not. * Use narrow includes in key backed types; remove some unnecessary return statements. * Fix snapshotFormatUnitTest infinite loop Patch addresses an issue where loop generating random buffer for the test might run in an infinite loop if KeySpace gets exhausted but the 'targetBuffer' bytes aren't generated. Description Testing * smaller function * Remove one more include and some extraneous return statements. Add a NullCodec that passes through the arguments unchanged. * Add a Tuple::makeTuple function to easily construct a tuple. Update Tuple to allow all types to be passed via .append() so they can be used with makeTuple. * Add the ability for StringRef.eat to return whether a separator was found. Add a serialize function and equality operators to ClusterConnectionString. Remove unused variable. * add unittest * Convert existing tuple usages to use Tuple::makeTuple() * resolve conflict upstream/main * Remove the ability to configure a tenant subspace. Rename the prefixes used for tenant metadata. * Fixing merge convergence bugs * Refactoring granule flush to retry properly on granule rollback * cleanup * Issue 7570: Check existance of container before creation attempt * cleanup * Use a constexpr check for deserialization (from code review) * Increase AllTags field length in TLogReady * Add a comment to name the fold expression syntax in makeTuple. This should make it easier to look up for those unfamiliar with the syntax. * Fix ASAN build (#7629) * Fix ASAN build Description -diff-2: Fix Mac build issues -diff-1: Address review comments Patch addresses the issue where ASAN build failed after introducing BlobGranule compression. Testing ASAN build * only do check after bootstraping test * Some minor changes from code review * solve review comments * function naming using intention rather than implementation details * update comments * Build fixes/improvements for sanitizers (#7657) * Don't build fdb c shim with ubsan This avoids duplicate symbols when linking. It doesn't really make sense to assemble files with -fsanitize=undefined anyway, since it won't insert instrumentation. * Consolidate boost_asan with boost_target * Log tlog initialization * Allow read range on null shard (#7654) * Fixed ShardedRocks unit test. * Removed `UNIT` from noSim/ShardedRocksDBTest.toml. Co-authored-by: He Liu <heliu@apple.com> * Validate subrange reads in simulation (#7597) * Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment * Add TraceEvent to the construction/destruction of DatabaseContext This is to figure out why huge databaseLogger related TraceEvents generated after 7.1 released. * Use key-backed types for tenants. Add a tenant state field that will be used in upcoming work. Some other tenant related refactoring. * Add CommitDebug event after Commit Proxy reporting committed version to master * fixup! Update code per comments * Ss shard management (#7340) * Storage server shard management with physical shards. * Cleanup. * Resolved comments. * Added `UnlimintedCommitBytes`. Co-authored-by: He Liu <heliu@apple.com> * Proxy GlobalConfig reads through GRV proxies Clients should avoid reading system keys unless authorized. Under global config, each client reads from the system keyspace to check for new global config keys. This commit moves these reads to a server role (the GRV proxies) and sends the results back to GlobalConfig for an in-memory update. * Batch global config refresh requests * Perform migration automatically on proxy boot * Update fdbserver/GrvProxyServer.actor.cpp Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com> * Update fdbclient/include/fdbclient/CommitProxyInterface.h Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com> * Add migration timeout * Reduce global configuration system key reads from proxy Clients now poll the proxy for the latest global config for a specific version. The proxy now periodically requests the latest global configuration data and stores it in memory, enabling it to respond immediately to clients with the appropriate version. * Add delay to allow for global config sync time * Mess with timeout values * Simplify refresh condition * Fix Tuple error * Update fdbserver/GrvProxyServer.actor.cpp Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com> * Update fdbserver/GrvProxyServer.actor.cpp Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com> * Add comments to deleteTenantTransaction. Fetch the tenant ID from the first transaction in deleteTransaction to use on subsequent retries. * Don't pass non-pod to variadic function (#7668) fdbserver/storageserver.actor.cpp:654:24: error: cannot pass object of non-trivial type 'const std::string' (aka 'const basic_string<char, char_traits<char>, allocator<char>>') through variadic function; call will abort at runtime [-Wnon-pod-varargs] Just use fmt instead * Disable guard pages for valgrind build. Closes #7610 (#7672) * fixup! Let clang accepts crc32_wrapper.c * Add timeout during Status generation Resolving hostname might be blocking. * Reduce a DNS lookup in Status generation * Update status json doc with fetch_storage_wiggler_stats_timeout error * Fix WinGDI name collision by defining macro NOGDI TenantState::ERROR collides with WinGDI's #define ERROR 0 * Move DD queue code over to using movement-reasons rather than priority (#7614) * Use enum variables to invoke Priority checking * add an explicit isDataMovementForReadBalancing function * Set up RelocateShard in terms of data movement reason instead of priority * Remove isMountainChopperPriority * Remove isDiskRebalancePriority * Fix formatting * Fix misnamed DataMovementReason::TEAM_HEALTHY Co-authored-by: Zhongxing Zhang <zhongxing.zhang@snowflake.com> * Error in FetchKey GRV shouldn't fail storageserver * Add testing mechanism * ddsketch_utility: Add utility scripts for ddsketch (#7602) * Adds ddsketch_calc.py which implements a class for DDSketch related calculations * Adds ddsketch_conversion.py for quickly computing a bucket index to a value or vice-versa * Adds ddsketch_compare.py to compute how similar two ddsketch distributions are * Adds export_graph.py to graph the ddsketch distribution outputted from mako The arguments for ddsketch_conversion.py are: -b, --bucket: the bucket index that we need to calculate the value from (optional) -v, --value: the value that we need to calculate the bucket index from (optional) -e, --error_guarantee: the error guarantee for ddsketch (optional, default is 0.005) The arguments for ddsketch_compare.py are: --file1: Path to first ddsketch json --file2: Path to second ddsketch json --txn1: The transaction type for the first file --txn2: The transaction type for the second file --op: The operation name (ex: GRV, GET ...) The arguments for export_graph.py: --file: path to ddsketch distribution --txn, -t: Transaction type from file --title: title for graph (optional, otherwise "Title" is used) --savefig: Path to save the image plot (optional) --op: Which operation to plot Co-authored-by: Xiaoxi Wang <xiaoxi.wang@snowflake.com> Co-authored-by: Josh Slocum <josh.slocum@snowflake.com> Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com> Co-authored-by: Ata E Husain Bohra <ata.husain@snowflake.com> Co-authored-by: Hari Bhaskaran <hbhaskaran@gmail.com> Co-authored-by: Trevor Clinkenbeard <trevor.clinkenbeard@snowflake.com> Co-authored-by: Zhe Wu <halfprice@users.noreply.github.com> Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com> Co-authored-by: He Liu <86634338+liquid-helium@users.noreply.github.com> Co-authored-by: He Liu <heliu@apple.com> Co-authored-by: Xiaoge Su <magichp@gmail.com> Co-authored-by: Jingyu Zhou <jingyuzhou@gmail.com> Co-authored-by: Lukas Joswiak <lukas.joswiak@snowflake.com> Co-authored-by: Junhyun Shim <junhyun.shim@snowflake.com> Co-authored-by: Bharadwaj V.R <bharadwaj.vr@snowflake.com> Co-authored-by: Zhongxing Zhang <zhongxing.zhang@snowflake.com> Co-authored-by: Markus Pilman <markus.pilman@snowflake.com> Co-authored-by: Kevin Hoxha <kevin.hoxha@snowflake.com>

* Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment

* Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com>

…#7778) * Validate subrange reads in simulation (#7597) * Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment * Add test coverage for SpecialKeyRangeAsyncImpl::getRange (#7671) * Add getRange test coverage for SpecialKeyRangeAsyncImpl * Fix the bug in SpecialKeyRangeAsyncImpl found by the test * Refactor ConflictingKeysImpl::getRange to use containedRanges to simplify the code * Fix file format * Initialize SpecialKeyRangeAsyncImpl cache with correct end key * Add release notes * Revert "Refactor ConflictingKeysImpl::getRange to use containedRanges to simplify the code" This reverts commit fdd298f. Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com>

* Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com>

…apple#7778) * Validate subrange reads in simulation (apple#7597) * Add extra validation to special key space reads in simulation * Fix bugs turned up by validating subrange reads * Change to validateSpecialSubrangeRead It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently. * Add comment * Add comment * Add test coverage for SpecialKeyRangeAsyncImpl::getRange (apple#7671) * Add getRange test coverage for SpecialKeyRangeAsyncImpl * Fix the bug in SpecialKeyRangeAsyncImpl found by the test * Refactor ConflictingKeysImpl::getRange to use containedRanges to simplify the code * Fix file format * Initialize SpecialKeyRangeAsyncImpl cache with correct end key * Add release notes * Revert "Refactor ConflictingKeysImpl::getRange to use containedRanges to simplify the code" This reverts commit fdd298f. Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com>

sfc-gh-anoyes requested review from sfc-gh-abeamon and sfc-gh-clin July 15, 2022 00:27

sfc-gh-anoyes marked this pull request as draft July 15, 2022 01:27

sfc-gh-clin mentioned this pull request Jul 20, 2022

Fix ConflictingKeysImpl::getRange #7643

Closed

5 tasks

sfc-gh-anoyes added 3 commits July 20, 2022 11:28

Add extra validation to special key space reads in simulation

cf3ff22

Fix bugs turned up by validating subrange reads

e5494a9

Change to validateSpecialSubrangeRead

4720b4c

It is in general not safe to expect that a read from the special key space returns the same results if performed again, since the transaction may be being modified concurrently.

sfc-gh-anoyes force-pushed the anoyes/special-key-reads-test branch from 0331b35 to 4720b4c Compare July 20, 2022 20:44

Add comment

3c4a437

sfc-gh-anoyes marked this pull request as ready for review July 20, 2022 21:33

sfc-gh-clin reviewed Jul 21, 2022

View reviewed changes

Comment thread fdbclient/SpecialKeySpace.actor.cpp

Add comment

de272ab

sfc-gh-clin self-requested a review July 21, 2022 19:00

sfc-gh-clin approved these changes Jul 21, 2022

View reviewed changes

sfc-gh-clin merged commit 89141d4 into apple:main Jul 21, 2022

sfc-gh-anoyes deleted the anoyes/special-key-reads-test branch July 21, 2022 21:43

sfc-gh-clin mentioned this pull request Jul 27, 2022

Validate subrange reads in simulation (#7597) (release-7.1) #7720

Merged

5 tasks

sfc-gh-clin mentioned this pull request Jul 27, 2022

Validate subrange reads in simulation (#7597) (snowflake-7.1) #7721

Merged

5 tasks

sfc-gh-clin mentioned this pull request Jul 27, 2022

Validate subrange reads in simulation (#7597) (release-7.0) #7723

Merged

5 tasks

sfc-gh-clin mentioned this pull request Jul 27, 2022

Validate subrange reads in simulation (#7597) (release-6.3) #7724

Merged

5 tasks

sfc-gh-clin added a commit that referenced this pull request Jul 29, 2022

Validate subrange reads in simulation (#7597) (release-7.0) (#7723)

a92b092

Uh oh!

Conversation

sfc-gh-anoyes commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code-Reviewer Section

For Release-Branches

Uh oh!

fdb-windows-ci commented Jul 15, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 15, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

sfc-gh-anoyes commented Jul 15, 2022

Uh oh!

foundationdb-ci commented Jul 15, 2022

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

sfc-gh-anoyes commented Jul 18, 2022

Uh oh!

fdb-windows-ci commented Jul 20, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 20, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

fdb-windows-ci commented Jul 20, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 20, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 20, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

Uh oh!

fdb-windows-ci commented Jul 21, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sfc-gh-anoyes commented Jul 15, 2022 •

edited

Loading