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

GetMappedRange support serializable & check RYW & continuation #6181

Merged
merged 5 commits into from
Mar 10, 2022

Conversation

nblintao
Copy link
Contributor

@nblintao nblintao commented Dec 22, 2021

This PR tries to fix 3 major caveats of the original "getRangeAndMap" implementation:

  • Support serializable
  • Support read-your-write (The transaction will fail if it actually reads what it wrote. This is not perfect but at least the semantic will be correct for the transactions that succeed.)
  • Support limits and continuation

To support this, the most important change is that the responses (at all layers) are no longer a flatten list of key-value pairs that contains the result of all underlying(or saying, secondary) queries,

but a list of objects, where each object contains

  • the key-value pair in the original range query
  • the begin and end key selector of the corresponding underlying query.
  • the list of key-value pairs that contains the the result of the underlying query.

In original design, the underlying query can be either a getRange or getValue, but this PR focuses on the case where they are getRange.

Also the naming is changed from "…AndMap" or "…AndFlatMap" to "Mapped…", in order to make is consistent and precise.

There are simulation tests, C binding unit tests, Java binding integration tests. All tests are updated to reflect the new features and new APIs.

20220203-235726-taolin-cb298d9cef410749 compressed=True data_size=21195353 duration=970376 ended=100000 fail_fast=1 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:24:22 sanity=False started=100053 stopped=20220204-002148 submitted=20220203-235726 timeout=5400 username=taolin

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 master 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)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS Catalina 10.15

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: d493067
  • Result: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: d493067
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from d493067 to cc4ec67 Compare January 11, 2022 22:46
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: cc4ec67
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS Catalina 10.15

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: cc4ec67
  • Result: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from cc4ec67 to 526132e Compare January 19, 2022 04:29
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 526132e
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS Catalina 10.15

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 526132e
  • Result: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from 526132e to ae3cbe0 Compare January 20, 2022 06:33
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: ae3cbe0
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from ae3cbe0 to 8d99e1a Compare January 25, 2022 23:59
@nblintao nblintao changed the title [WIP] GetRangeAndMap support serializable & check RYW [WIP] GetRangeAndMap support serializable & check RYW & continuation Jan 25, 2022
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 8d99e1a
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from 8d99e1a to acbe336 Compare February 4, 2022 00:34
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: acbe336
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@nblintao
Copy link
Contributor Author

nblintao commented Feb 4, 2022

I separated the part that I have finished as a separate commit 14787a658341d93eece4bdeba3f664d8477bcee4. It includes all the changes needed in NativeAPI layer and below (SS, for example). It also updates the GetRangeAndMap simulation test cover the changes.

20220203-235726-taolin-cb298d9cef410749 compressed=True data_size=21195353 duration=970376 ended=100000 fail_fast=1 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:24:22 sanity=False started=100053 stopped=20220204-002148 submitted=20220203-235726 timeout=5400 username=taolin

To reviewers: You can start reviewing that commit, so that there won't be surprises when the entire PR is ready. Please feel free let me know if you need more explanation or a walk-through of the code.

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from acbe336 to 66f5f40 Compare February 8, 2022 01:45
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 66f5f40
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 66f5f40
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from 66f5f40 to 8d15e58 Compare February 10, 2022 05:03
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 8d15e58
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 8d15e58
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from 8d15e58 to 2443058 Compare February 16, 2022 02:01
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 2443058
  • Result: FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)


MappedReqAndResultRef reqAndResult;

MappedKeyValueRef() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: use default constructor

MappedKeyValueRef() = default;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*
* This source file is part of the FoundationDB open source project
*
* Copyright 2013-2018 Apple Inc. and the FoundationDB project authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/2018/2022/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

bindings/c/test/unit/unit_tests.cpp Outdated Show resolved Hide resolved
fdbserver/storageserver.actor.cpp Outdated Show resolved Hide resolved
fdbserver/workloads/GetMappedRange.actor.cpp Outdated Show resolved Hide resolved
#include "flow/Error.h"
#include "flow/IRandom.h"
#include "flow/flow.h"
#include "ApiWorkload.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include "ApiWorkload.h"
#include "fdbserver/workloads/ApiWorkload.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// TODO: When n is large, split into multiple transactions.
state Transaction tr(cx);
try {
tr.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this line, and move line 100 out of the loop. Basically tr.onError does the right thing for resetting the internal states of tr and updating the timeout value.

If calling reset here, the timeout value is also reset, which is undesirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// TODO: When n is large, split into multiple transactions.
state Transaction tr(cx);
try {
tr.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete. And add a loop outside try...catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 1179 to 1182
static void addConflictRangeAndMustUnmodified(ReadYourWritesTransaction* ryw,
GetMappedRangeReq<backwards> read,
WriteMap::iterator* it,
MappedRangeResult* result) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this comment block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted.

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from c1a1515 to 5c24094 Compare March 9, 2022 01:26
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 5c24094
  • Result: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} if [ -d "foundationdb" ] && pgrep -x clang > /dev/null; then echo "There is a running macOS CI, please try again later."; exit 1; fi. Reason: exit status 2
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 5c24094
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: d884f3a
  • Result: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} if [ -d "foundationdb" ] && pgrep -x clang > /dev/null; then echo "There is a running macOS CI, please try again later."; exit 1; fi. Reason: exit status 2
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: d884f3a
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

Copy link
Contributor

@jzhou77 jzhou77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wait(tr.onError(e));
loop {
try {
tr.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this line.

@nblintao nblintao force-pushed the serializable-get-range-and-map branch from d884f3a to 4e40bf3 Compare March 9, 2022 22:46
jzhou77
jzhou77 previously approved these changes Mar 9, 2022
Copy link
Contributor

@jzhou77 jzhou77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

A suggestion is to move "mapper" logic, e.g., constructMappedKey, into its own files, which can be done in a separate PR.

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 4e40bf3
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: c866c21
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: c866c21
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@nblintao nblintao requested a review from jzhou77 March 10, 2022 01:03
@nblintao nblintao removed the request for review from halfprice March 10, 2022 18:05
@nblintao nblintao merged commit e2c7c30 into apple:main Mar 10, 2022
@nblintao nblintao deleted the serializable-get-range-and-map branch March 10, 2022 18:05
nblintao added a commit to nblintao/foundationdb that referenced this pull request Mar 11, 2022
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.

4 participants