-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
AWS CodeBuild CI Report for macOS Catalina 10.15
|
AWS CodeBuild CI Report for Linux CentOS 7
|
d493067
to
cc4ec67
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for macOS Catalina 10.15
|
cc4ec67
to
526132e
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for macOS Catalina 10.15
|
526132e
to
ae3cbe0
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
ae3cbe0
to
8d99e1a
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
Doxense CI Report for Windows 10
|
8d99e1a
to
acbe336
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
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.
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. |
Doxense CI Report for Windows 10
|
acbe336
to
66f5f40
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
66f5f40
to
8d15e58
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
8d15e58
to
2443058
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
fdbclient/FDBTypes.h
Outdated
|
||
MappedReqAndResultRef reqAndResult; | ||
|
||
MappedKeyValueRef() {} |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2018/2022/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
bindings/java/src/main/com/apple/foundationdb/MappedRangeResultDirectBufferIterator.java
Outdated
Show resolved
Hide resolved
bindings/java/src/main/com/apple/foundationdb/MappedRangeResult.java
Outdated
Show resolved
Hide resolved
#include "flow/Error.h" | ||
#include "flow/IRandom.h" | ||
#include "flow/flow.h" | ||
#include "ApiWorkload.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "ApiWorkload.h" | |
#include "fdbserver/workloads/ApiWorkload.h" |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
fdbclient/ReadYourWrites.actor.cpp
Outdated
static void addConflictRangeAndMustUnmodified(ReadYourWritesTransaction* ryw, | ||
GetMappedRangeReq<backwards> read, | ||
WriteMap::iterator* it, | ||
MappedRangeResult* result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this comment block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
c1a1515
to
5c24094
Compare
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for Linux CentOS 7
|
There was a problem hiding this 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this line.
d884f3a
to
4e40bf3
Compare
There was a problem hiding this 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.
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for Linux CentOS 7
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
AWS CodeBuild CI Report for Linux CentOS 7
|
This PR tries to fix 3 major caveats of the original "getRangeAndMap" implementation:
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
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.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branch
ormaster
if this is the youngest branch)