[Compatibility] Added GETEX command #695
Conversation
|
Fixed the current review comments |
|
|
||
| if (input.ExtraMetadata > 0) | ||
| { | ||
| byte* pbOutput = stackalloc byte[ObjectOutputHeader.Size]; |
There was a problem hiding this comment.
It's to call existing EvaluateExpireInPlace method but I don't care about the output param, so created a temp SpanByteAndMemory without any allocation and called the existing method
| case RespCommand.GETEX: | ||
| CopyRespTo(ref value, ref output); | ||
|
|
||
| if (input.ExtraMetadata > 0) |
There was a problem hiding this comment.
Shouldn't you check for !isPersist here?
There was a problem hiding this comment.
I don't have to because if there is ExtraMetadata then that means the user has provided the expiration time so it can't be a Persist options, so there is no point in checking for !isPersist
| } | ||
|
|
||
| oldValue.AsReadOnlySpan().CopyTo(newValue.AsSpan()); | ||
| var isPersist = *(bool*)(inputPtr + RespInputHeader.Size); |
There was a problem hiding this comment.
Is this necessary? If we're only doing GET + PERSIST, would we ever reach CopyUpdate?
There was a problem hiding this comment.
Ideally not, add it just in case. I am doubting myself mainly because the existing PERSIST comment also has an entry in CopyUpdater. if you are sure then I can remove it. Shall I?
| if (input.ExtraMetadata > 0) | ||
| { | ||
| Debug.Assert(newValue.Length == oldValue.Length + input.MetadataSize); | ||
| byte* pbOutput = stackalloc byte[ObjectOutputHeader.Size]; |
| @@ -185,6 +185,13 @@ public int GetRMWModifiedValueLength(ref SpanByte t, ref SpanByte input) | |||
| // No additional allocation needed. | |||
| break; | |||
|
|
|||
| case RespCommand.GETEX: | |||
| if (input.ExtraMetadata > 0) | |||
There was a problem hiding this comment.
Assuming this should always be true if we reach here? Could just do a Debug.Assert(input.ExtraMetadata > 0)
|
@TalZaccai Fixed the review comments, there is one open question there. |
Adding GETEX command to garnet