Add transaction getApproximateSize() API #1756
Conversation
jzhou77
force-pushed
the
db-option
branch
3 times, most recently
from
5b8e779 to
f232158
Compare
|
@fdb-build test this please |
jzhou77
force-pushed
the
db-option
branch
2 times, most recently
from
9b6f9a1 to
7f193da
Compare
bindings/java/src/test/com/apple/foundationdb/test/AsyncStackTester.java
Outdated
Show resolved
Hide resolved
|
@fdb-build test this please |
1 similar comment
|
@fdb-build test this please |
The size is the summation of expected size of mutations, read conflict ranges, and write conflict ranges.
Change return type to int64_t and fix C and Python binding to use the correct type.
Both key and value has to be of type bytes.
|
100k binding correctness tests passed. |
ajbeamon
left a comment
ajbeamon
left a comment
There was a problem hiding this comment.
I might be missing it, but it looks like the accounting for reads and atomic ops hasn't been added.
fdbclient/ReadYourWrites.actor.cpp
Outdated
| if(options.readYourWritesDisabled ) { | ||
| if (options.readYourWritesDisabled) { | ||
| approximateSize += key.expectedSize() + value.expectedSize() + sizeof(MutationRef) + | ||
| (addWriteConflict ? sizeof(KeyRangeRef) : 0); |
There was a problem hiding this comment.
If we are accounting for a conflict range here, I think we may also need to add 2 * key.expectedSize() + 1 for the keys in the range.
Also, do we need to add the conflict range cost here regardless of whether we are bypassing RYW? It looks like the conflict ranges aren't added until commit time (or possibly during usage of versionstamps), so we would probably also need to add a parameter to the conflict range adding functions to avoid updating the approximate size when called from those contexts.
There was a problem hiding this comment.
For read conflict ranges, add in ReadYourWritesTransaction::updateConflictMap or direct add for readYourWritesDisabled case.
For write conflict ranges, don't add their size in the commit time by adding a flag to ReadYourWritesTransaction::writeRangeToNativeTransaction. The idea is that, after commit, client code should call Transaction::getSize() to get transaction size.
fdbclient/ReadYourWrites.actor.cpp
Outdated
| throw key_outside_legal_range(); | ||
|
|
||
| if( options.readYourWritesDisabled ) { | ||
| approximateSize += range.expectedSize() + sizeof(MutationRef) + (addWriteConflict ? sizeof(KeyRangeRef) : 0); |
There was a problem hiding this comment.
Same question about conflict range.
fdbclient/ReadYourWrites.actor.cpp
Outdated
| if (key.size() > (key.startsWith(systemKeys.begin) ? CLIENT_KNOBS->SYSTEM_KEY_SIZE_LIMIT : CLIENT_KNOBS->KEY_SIZE_LIMIT)) | ||
| return key_too_large(); | ||
|
|
||
| approximateSize += key.expectedSize() + sizeof(Watch); |
There was a problem hiding this comment.
Looking into this, it appears the cost of a watch in the commit request is one read conflict range. That means that you shouldn't add anything to size unless we are bypassing RYW, in which case you should add the cost of the conflict range.
Co-Authored-By: A.J. Beamon <[email protected]>
Use fdb_future_get_int64 for language bindings and get rid of using Version with getApproximateSize API.
Use fdb_future_get_int64 in all bindings.
Because different bindings may issue different limit for get_range calls, it is impossible to return the same size value for getApproximateSize API. So we just push a string to make sure binding test results are the same. Use another unit test to make sure the sizes got back are monotonically increasing.
|
Addressed all comments and 100k binding tests passed. @ajbeamon can you take one more look? Thanks! |
ajbeamon
left a comment
ajbeamon
left a comment
There was a problem hiding this comment.
FutureVersion type in Python can be removed as well.
ajbeamon
left a comment
ajbeamon
left a comment
There was a problem hiding this comment.
We should also add some release notes for this. I think we'll probably need at least 2, one to describe the new feature and another to note that fdb_future_get_version has been renamed.
Co-Authored-By: A.J. Beamon <[email protected]>
Co-Authored-By: A.J. Beamon <[email protected]>
Mutations in writeRangeToNativeTransaction() is already counted, so there is no need to count them again.
|
Fixed all review comments and passed 100k binding tests. |
|
|
||
| int64_t version; | ||
| checkError(fdb_future_get_version(f, &version), "get version", rs); | ||
| checkError(fdb_future_get_int64(f, &version), "get version", rs); |
There was a problem hiding this comment.
Same API version problem here.
Co-Authored-By: A.J. Beamon <[email protected]>
Only use fdb_future_get_int64 for API version >= 620.
This was missed from original PR apple#1756, where only C API was documented.
This was missed from original PR apple#1756, where only C API was documented.
5 participants
The size is the estimation of expected size of mutations, read conflict ranges, and write conflict ranges.
After the commit, the client may use Transaction::getSize() to get the exact commit size.
These APIs are available in version 620.
fdb_future_get_versionis deprecated in version 620, i.e., 6.2 release. Instead, usefdb_future_get_int64for the same behavior. All bindings will usefdb_future_get_int64at version 620 and above.This fixes #1682.