-
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
Return error when getRangeAndFlatMap has more & Improve simulation tests #6029
Conversation
AWS CodeBuild CI Report for Linux CentOS 7
|
9873658
to
1b57cc5
Compare
AWS CodeBuild CI Report for macOS Catalina 10.15
|
AWS CodeBuild CI Report for Linux CentOS 7
|
1b57cc5
to
9482bea
Compare
AWS CodeBuild CI Report for macOS Catalina 10.15
|
AWS CodeBuild CI Report for Linux CentOS 7
|
@@ -895,64 +895,78 @@ static std::string recordValue(const int i) { | |||
return Tuple().append(dataOfRecord(i)).pack().toString(); | |||
} | |||
|
|||
TEST_CASE("fdb_transaction_get_range_and_flat_map") { | |||
std::map<std::string, std::string> fillInRecords(int n) { |
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.
Is the return value used anywhere?
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.
No, but we may want to use it in the future.
return data; | ||
} | ||
|
||
GetRangeResult getIndexEntriesAndMap(int beginId, int endId, fdb::Transaction& tr) { |
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: change tr
to a pointer to indicate that it is mutable. Mutable reference is hard to read.
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.
tr
here is immutable. I'm using reference to keep get_range_and_flat_map
consistent with existing functions like GetRangeResult get_range(fdb::Transaction& tr
bindings/c/test/unit/unit_tests.cpp
Outdated
// result[1]: key=fdbRECORDprimary-key-of-record-00000001, value=data-of-record-00000001 | ||
CHECK(recordKey(i).compare(key) == 0); | ||
CHECK(recordValue(i).compare(value) == 0); | ||
// std::cout << "result[" << i << "]: key=" << key << ", value=" << value << std::endl; |
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.
Remove this debugging output?
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.
Removed.
fdbserver/storageserver.actor.cpp
Outdated
ACTOR Future<Optional<Value>> quickGetValue(StorageServer* data, | ||
StringRef key, | ||
Version version, | ||
// To provide some contexts. |
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.
Can you extend this comment a bit more? Currently, it doesn't provide any information to understand why this is needed.
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.
Good point. Changed to To provide span context, tags, debug ID to underlying lookups.
|
||
data->actors.add(data->readGuard(req, getKeyValuesQ)); |
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.
What's the issue with this being guarded?
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.
// Note that it does not use readGuard to avoid server being overloaded here. Throttling is enforced at the
// original request level, rather than individual underlying lookups. The reason is that throttle any
// individual underlying lookup will fail the original request, which is not productive.
Guard is used to throttle requests. We already use guard at the original request. If we guard every underlying lookups, it is very likely one of them are throttled and then the entire request is failed, which wastes all successful underlying lookups.
if (!reply.error.present()) { | ||
++data->counters.quickGetValueHit; | ||
return reply.value; | ||
} |
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.
Is the error here interesting or normal? Do you want to count how many of them are happened?
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.
Error here is normal. For example, it will hit error if the requested key is not in the shard. All errors are counted at ++data->counters.quickGetValueMiss;
below.
std::cout << "result.size()=" << result.size() << std::endl; | ||
std::cout << "result.more=" << result.more << std::endl; | ||
ASSERT(result.size() == endId - beginId); | ||
const KeyValueRef* it = result.begin(); |
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.
why not put this in the for loop?
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.
I thought FLOW didn't like it. But it seems working now. Fixed.
before we are able to support continuation
- Improve simulation test to be more robust and verify more things. - Renamed the simulation test from `IndexPrefetchDemo` to `GetRangeAndMap`. - Remove throttling the individual lookups underlying a getRangeAndFlatMap request. - Fix error handlings in `quickGetKeyValues` and `quickGetValue`. - Turn `QUICK_GET_VALUE_FALLBACK` and `QUICK_GET_KEY_VALUES_FALLBACK` off by default. - Propagate contexts to underlying lookups.
9482bea
to
bbb0942
Compare
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!
IndexPrefetchDemo
toGetRangeAndMap
.quickGetKeyValues
andquickGetValue
.QUICK_GET_VALUE_FALLBACK
andQUICK_GET_KEY_VALUES_FALLBACK
off by default.Ran 100k for
GetRangeAndMap
alone.20211130-173956-taolin-92b2ee2b1c127e25 compressed=True data_size=22400913 duration=1298112 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:25:07 sanity=False started=100117 stopped=20211130-180503 submitted=20211130-173956 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)