-
Notifications
You must be signed in to change notification settings - Fork 520
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
Etag Implementation #675
base: main
Are you sure you want to change the base?
Etag Implementation #675
Conversation
77487f1
to
d564238
Compare
b126e5e
to
da48417
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.
Not finished with review yet but sending a few comments now
BenchmarkingEtag Impl Branch - Commit 141c870BDN
RESPGET<<<<<<< Benchmark Configuration >>>>>>>>
|
Method | Mean | Error | StdDev | Median | Gen0 | Allocated |
---|---|---|---|---|---|---|
InlinePing | 1.766 us | 0.0260 us | 0.0243 us | 1.767 us | - | - |
Set | 13.370 us | 0.1537 us | 0.1438 us | 13.444 us | - | - |
SetEx | 19.677 us | 0.3127 us | 0.2925 us | 19.740 us | - | - |
Get | 10.117 us | 0.1089 us | 0.1019 us | 10.140 us | - | - |
Increment | 22.060 us | 0.3324 us | 0.3110 us | 22.132 us | - | - |
ZAddRem | 110.356 us | 0.5530 us | 0.4902 us | 110.402 us | 0.3662 | 23552 B |
LPushPop | 103.980 us | 1.3591 us | 1.2713 us | 104.172 us | 0.2441 | 18432 B |
SAddRem | 96.300 us | 0.8453 us | 0.7494 us | 96.195 us | 0.2441 | 16384 B |
HSetDel | 156.368 us | 4.4619 us | 12.4380 us | 156.181 us | - | 55297 B |
MyDictSetGet | 213.157 us | 6.9432 us | 20.2536 us | 207.258 us | 0.4883 | 30720 B |
RESP
GET
<<<<<<< Benchmark Configuration >>>>>>>>
benchmarkType: Throughput
skipLoad: Disabled
DBsize: 1024
TotalOps: 33554432
Op Benchmark: GET
KeyLength: 1
ValueLength: 8
BatchSize: 4096
RunTime: 15
NumThreads: 1,8,16,32
Auth:
Load Using SET: Disabled
ClientType used for benchmarking: LightClient
TLS: Disabled
ClientHistogram: False
minWorkerThreads: 1000
minCompletionPortThreads: 1000
Generating 8 MSET request batches of size 128 each; total 1024 ops
Request generation complete
maxBytesWritten out of maxBufferSize: 2984/65536
Loading time: 0.002 secs
Operation type: MSET
Num threads: 8
Total time: 21.00ms for 1,024.00 ops
Throughput: 48,761.90 ops/sec
Loaded 1024 keys into the DB >>>
Generating 8192 GET request batches of size 4096 each; total 33554432 ops
Resizing request buffer from 65536 to 131072
Request generation complete
maxBytesWritten out of maxBufferSize: 89851/131072
Loading time: 2.322 secs
Operation type: GET
Num threads: 1
Total time: 15,004.00ms for 101,146,624.00 ops
Throughput: 6,741,310.58 ops/sec
Operation type: GET
Num threads: 8
Total time: 15,010.00ms for 276,013,056.00 ops
Throughput: 18,388,611.33 ops/sec
Operation type: GET
Num threads: 16
Total time: 15,005.00ms for 300,077,056.00 ops
Throughput: 19,998,470.91 ops/sec
Operation type: GET
Num threads: 32
Total time: 15,008.00ms for 289,550,336.00 ops
Throughput: 19,293,066.10 ops/sec
SET
<<<<<<< Benchmark Configuration >>>>>>>>
benchmarkType: Throughput
skipLoad: Disabled
DBsize: 1024
TotalOps: 33554432
Op Benchmark: SET
KeyLength: 1
ValueLength: 8
BatchSize: 4096
RunTime: 15
NumThreads: 1,8,16,32
Auth:
Load Using SET: Disabled
ClientType used for benchmarking: LightClient
TLS: Disabled
ClientHistogram: False
minWorkerThreads: 1000
minCompletionPortThreads: 1000
Generating 8 MSET request batches of size 128 each; total 1024 ops
Request generation complete
maxBytesWritten out of maxBufferSize: 2984/65536
Loading time: 0.002 secs
Operation type: MSET
Num threads: 8
Total time: 85.00ms for 1,024.00 ops
Throughput: 12,047.06 ops/sec
Loaded 1024 keys into the DB >>>
Generating 8192 SET request batches of size 4096 each; total 33554432 ops
Resizing request buffer from 65536 to 131072
Resizing request buffer from 131072 to 262144
Request generation complete
maxBytesWritten out of maxBufferSize: 147195/262144
Loading time: 5.168 secs
Operation type: SET
Num threads: 1
Total time: 15,015.00ms for 114,081,792.00 ops
Throughput: 7,597,854.95 ops/sec
Operation type: SET
Num threads: 8
Total time: 15,005.00ms for 247,541,760.00 ops
Throughput: 16,497,284.91 ops/sec
Operation type: SET
Num threads: 16
Total time: 15,021.00ms for 259,411,968.00 ops
Throughput: 17,269,953.27 ops/sec
Operation type: SET
Num threads: 32
Total time: 15,012.00ms for 249,937,920.00 ops
Throughput: 16,649,208.63 ops/sec
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.
reviewed through MainStoreOps.cs; Tsavorite.core TBD
libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/InternalUpsert.cs
Show resolved
Hide resolved
Debug.Assert(numItemInArr == 2, "GETWITHETAG output RESP array should be of 2 elements only."); | ||
|
||
// we know a key-val pair does not have an etag if the first element is not null | ||
// if read nil is successful it will point to the ptrVal otherwise we need to re-read past the etag as RESP int64 |
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.
These comments don't seem to align with the code; should they be:
// We know a key-val pair has an etag if the first element is not null
// If REadMo; is successful it will increment outputBufCurr to point to the ptrVal; otherwise we need to Read64Into to move outputBufCurr past the etag
bool etagReadingSuccessful = RespReadUtils.Read64Int(out _, ref startOfEtagPtr, end); | ||
Debug.Assert(etagReadingSuccessful, "Etag should have been read succesffuly"); | ||
// now startOfEtagPtr is past the etag and points to value | ||
outputBufCurr = startOfEtagPtr; |
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 simply pass "ref outputBufCurr" on Read64Int? It'd also leave startOfETagPtr where it would be, which would reduce surprise if it's used elsewhere. (It's not, here, and in some cases I use {} to create a temp scope just to have these very-local variables scoped as minimally as possible)
@@ -695,68 +709,75 @@ private unsafe GarnetStatus RENAME(ArgSlice oldKeySlice, ArgSlice newKeySlice, S | |||
{ |
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.
this "hasETag" must always be true now, after your change above to make this the "else" of "!hasETag"
@@ -0,0 +1,109 @@ | |||
--- |
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.
We have a section for Garnet specific commands here, perhaps move this documentation there? Or link to it? https://microsoft.github.io/garnet/docs/commands/garnet-specific-commands
@@ -219,6 +219,7 @@ Set **key** to hold the string value. If key already holds a value, it is overwr | |||
* NX -- Only set the key if it does not already exist. | |||
* XX -- Only set the key if it already exists. | |||
* KEEPTTL -- Retain the time to live associated with the key. | |||
* RETAINETAG -- Update the Etag associated with the previous key-value pair, while setting the new value for the key. If no etag existed on the previous key-value pair this will create the new key-value pair without any etag as well. |
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.
call out that this is Garnet-specific, and link to the ETag documentation here.
ETag Support
Garnet provides support for ETags on raw strings. By using the ETag-related commands outlined below, you can associate any string-based key-value pair inserted into Garnet with an automatically updated ETag.
Compatibility with non-ETag commands and the behavior of data inserted with ETags are detailed at the end of this document.
SETWITHETAG
Syntax
Inserts a key-value string pair into Garnet, associating an ETag that will be updated upon changes to the value.
Options:
Response
GETWITHETAG
Syntax
Retrieves the value and the ETag associated with the given key.
Response
One of the following:
SETIFMATCH
Syntax
Updates the value of a key if the provided ETag matches the current ETag of the key.
Response
One of the following:
GETIFNOTMATCH
Syntax
Retrieves the value if the ETag associated with the key has changed; otherwise, returns a response indicating no change.
Response
One of the following:
Compatibility and Behavior with Non-ETag Commands
Below is the expected behavior of ETag-associated key-value pairs when non-ETag commands are used.
MSET, BITOP: These commands will replace an existing ETag-associated key-value pair with a non-ETag key-value pair, effectively removing the ETag.
SET: Only if used with additional option "RETAINETAG" will calling SET update the etag while inserting the new key-value pair over the existing key-value pair.
RENAME: Renaming an ETag-associated key-value pair will reset the ETag to 0 for the renamed key. Unless the key being renamed to already existed before hand, in that case it will retain the etag of the existing key that was the target of the rename.
All other commands will update the etag internally if they modify the underlying data, and any responses from them will not expose the etag to the client. To the users the etag and it's updates remain hidden in non-etag commands.