add bytelimit for prefetch with better granularity#8756
Conversation
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS BigSur 11.5.2
|
Result of foundationdb-pr-macos on macOS Monterey 12.x
|
|
seems there are some testing issue, will take a look tomorrow |
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Doxense CI Report for Windows 10
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Doxense CI Report for Windows 10
|
d0276ea to
66c7210
Compare
This change adds bytelimit for prefetch API inside fdbserver, in order to do that, a better granularity is provided when fetching indexes and record. Instead of fetching index once for each prefetch call, now there might be multiple index fetches.
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Doxense CI Report for Windows 10
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Monterey 12.x
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS BigSur 11.5.2
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Doxense CI Report for Windows 10
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
alecgrieser
left a comment
There was a problem hiding this comment.
LGTM, though I'm not an expert on this code. To summarize, the major changes are:
- During the scan of the primary range, a new "page size" parameter is added which breaks up the scan into multiple fetches
- The mapping of keys to other keys now checks the remaining bytes and stops executing batches of secondary fetches if the byte limit is exceeded. The batch size default is also decreased from 50 to 10.
Overall, this should result in getting requests to be closer to the configured byte limit, as the amount of potential overrun is limited to one page's worth of the primary range (from step 1) plus one batch's worth of secondary fetches (from step 2)
| // fill index KV for open boundary | ||
| if (firstBatch && i + offset == 0) { | ||
| result->data[0].key = input.data[0].key; | ||
| result->data[0].value = input.data[0].value; | ||
| } |
There was a problem hiding this comment.
I'm not sure I understand what this if block is trying to do. Why does result->data[0] need to be filled in in this case?
There was a problem hiding this comment.
we provide the index key for both the begin and the end of the range, so that client can use them as continuations -- I think it is because we might scan forward/backward, so we add both. Here it is the beginning.
| throw; | ||
| // these fields always return as the value of the last batch result | ||
| result.version = getKeyValuesReply.version; | ||
| result.cached = getKeyValuesReply.cached; |
There was a problem hiding this comment.
What does this cached value represent? Is it correct to always set it to the last one?
There was a problem hiding this comment.
we can remove it, because I do not think it is used anywhere, @ohadzeliger can you please verify that RL has not been using this field?
There was a problem hiding this comment.
oh i mean the value of this field might not used in prefetch code path, not deleting this field from code
There was a problem hiding this comment.
cahced is not exposed through the Java bindings, we don't see it.
| @@ -4602,52 +4602,59 @@ ACTOR Future<Void> mapSubquery(StorageServer* data, | |||
| Arena* pArena, | |||
| int matchIndex, | |||
| bool isRangeQuery, | |||
There was a problem hiding this comment.
Does removing isBoundary make the protocol level change?
There was a problem hiding this comment.
two things:
- removing the field in java/c bindings might cause issues, and we should be careful about that.
- but this value is not being used anyway, and this field here is fdbserver internal, so my plan is to not setting it until we remove it. I can add a comments like "to be deleted" in that field
| init( STRICTLY_ENFORCE_BYTE_LIMIT, false); // If set to false, prefetch response might exceeed byteLimit as records are fetched in batch | ||
| // tentatively use the limit of secondary queries so that 1 primary query serves one batch | ||
| // we can increase this to gain better performance after proper benchmarking | ||
| init( PREFETCH_INDEX_PAGE_SIZE, 20 ); if ( randomize && BUGGIFY ) PREFETCH_INDEX_PAGE_SIZE = deterministicRandom()->randomInt(50, 100); |
There was a problem hiding this comment.
PREFETCH_INDEX_PAGE_SIZE cannot be controlled by the client API, right?
There was a problem hiding this comment.
right, that needs a protocol change, and I am not sure if want to expose that anyways.
| Key endTuple = Tuple::makeTuple(prefix, INDEX, indexKey(endId)).getDataAsStandalone(); | ||
| state KeySelector endSelector = KeySelector(firstGreaterOrEqual(endTuple)); | ||
| state int limit = 100; | ||
| state int byteLimit = deterministicRandom()->randomInt(1, 9) * 10000; |
There was a problem hiding this comment.
What is the purpose of this (why is it random)?
There was a problem hiding this comment.
because when byteLimit is large, we want to test up to(but not exceeding) limit rows can be returned, when it is small, we want to test fewer than limit rows can be returned. So both cases need to be tested.
Doxense CI Report for Windows 10
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
|
check #8768 instead |
This change adds bytelimit for prefetch API inside fdbserver, in order to do that, a better granularity is provided when fetching indexes and record.
Instead of fetching index once for each prefetch call, now there might be multiple index fetches.
Replace this text with your description here...
Code-Reviewer Section
The general pull request 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-branchormainif this is the youngest branch)