-
Notifications
You must be signed in to change notification settings - Fork 632
[WIP] New ETag Commands Syntax + ETag Object Support #1478
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
base: dev
Are you sure you want to change the base?
Conversation
… add tests and fixes for OversizePages
…start of LogRecord-related docs
…changes; update LogRecord docs
…ld not need to know about the length prefix (especially bc it changes between Inline, Overflow, ObjectId)
Add YCSB --di (Delete and reInsert) option
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.
Pull request overview
This PR introduces a new "meta command" architecture for ETag operations in Garnet. Instead of separate commands like GETWITHETAG, SETIFMATCH, etc., the PR implements wrapper commands (EXECWITHETAG, EXECIFMATCH, EXECIFNOTMATCH, EXECIFGREATER) that can envelop any data command. This provides a more unified and extensible approach to ETag handling across both main store and object store operations.
Key changes include:
- Introduction of meta command parsing infrastructure that supports nested command execution
- Replacement of standalone ETag commands with meta command wrappers
- Extension of ETag support to object store operations (particularly sorted sets)
Reviewed changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test/*.cs | Updated test files to use new meta command syntax (e.g., EXECWITHETAG SET instead of SET...WITHETAG) |
| libs/server/Resp/Parser/SessionParseState.cs | Added meta argument tracking with separate buffer pointers and count |
| libs/server/Resp/Parser/RespCommand.cs | Implemented meta command parsing with nested command detection |
| libs/server/Storage/Functions/**/VarLenInputMethods.cs | Updated field info methods to use MetaCmd.IsEtagCommand() instead of flag checks |
| libs/server/Storage/Functions/**/RMWMethods.cs | Refactored ETag handling to use GetUpdatedEtag() utility and support conditional execution |
| libs/server/Storage/Functions/SessionFunctionsUtils.cs | Added helper methods for ETag computation and conditional execution checks |
| playground/CommandInfoUpdater/*.json | Updated command metadata to reflect removed/added commands |
| samples/ETag/*.cs | Updated sample code to use new meta command syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <param name="sizeChange"></param> | ||
| /// <returns></returns> | ||
| bool Operate(ref ObjectInput input, ref ObjectOutput output, byte respProtocolVersion, out long sizeChange); | ||
| bool Operate(ref ObjectInput input, ref ObjectOutput output, byte respProtocolVersion, bool execOp, long updatedEtag, out long sizeChange); |
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.
add comments on what execOp and updatedEtag mean. ideally Operate and calls like it should not accept arguments about etags specifically. they just receive a meta command and its arguments.
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 agree, that irks me too, the issue here is that Operate sets the output of the command which may need to include the etag (and there are different scenarios depending on the Operate logic, for instance it may need to only output an error without the etag even when one is requested).
libs/server/InputHeader.cs
Outdated
| /// <param name="metaCmd">Meta command</param> | ||
| /// <param name="flags">Flags</param> | ||
| public RespInputHeader(RespCommand cmd, RespInputFlags flags = 0) | ||
| public RespInputHeader(RespCommand cmd, RespMetaCommand metaCmd, RespInputFlags flags = 0) |
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.
meta command could be 0 by default, to simplify the common cases where nobody cares about meta commands.
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 wanted to err on the side of caution here, I didn't want it to be easy to just forget to input the meta command. If you want to ignore it, you should do so explicitly.
libs/server/Resp/BasicCommands.cs
Outdated
| : TimeSpan.FromSeconds(expiry).Ticks); | ||
|
|
||
| var input = new StringInput(cmd, ref parseState, startIdx: 1, arg1: inputArg); | ||
| var input = new StringInput(cmd, metaCommand, ref parseState, startIdx: 1, arg1: inputArg); |
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.
it is unclear whether parseState here contains the wrapping command as well. ideally we have a parse state for the actual command that is no different from before, and an optional extra parse state for the outer wrapper shell.
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.
parseState contains both, and they are seperately accessed. The meta-parameters preceed the "standard" parameters on the buffer. You would access the standard parameters as you would before (indexes unchanged) and you access the meta-parameters by explicitly specifying isMetaArg: true.
<<This PR is still a work in progress>>
This PR introduces a notion of "meta commands" - commands that "envelop" existing data commands.
The meta commands introduced in this PR are ETag related - EXECWITHETAG (execute a command and add the dest key's etag to the output), as well as EXECIFMATCH, EXECIFNOTMATCH, EXECIFGREATER (conditionally-execute a command based on the dest key's ETag in relation to the ETag meta-argument).
These command are able to replace the existing etag-related commands and command arguments, and in-turn support many more (including object commands).
For example:
DELIFGREATER key etag->EXECIFGREATER etag DEL keyGETIFNOTMATCH key etag->EXECIFNOTMATCH etag GET keyGETWITHETAG key->EXECWITHETAG GET keySETIFMATCH key value etag->EXECIFMATCH etag SET key valueTo achieve this logic, we parse the meta command and its meta-arguments into the
SessionParseState. Currently this logic supports a constant number of meta-arguments which is defined in theArityproperty in the meta-command'sRespCommandInfo(arg count = arity - 1).When the parse state gets handed to the appropriate
ISessionFunctionsmethod, the method decides whether the operation should get executed (in the case of conditional-execution) and to what value should the record's ETag advance (if at all).The "special output" (in case the ETag needs to be written along with the command output) is handled by the
ISessionFunctionsmethod in the main and unified case, and in theIGarnetObject.Operatemethod in the object case.To-do:
SessionParseStateto parse and hold meta-argumentsInputHeaderto include meta-command and update commandsISessionFunctionsfor all 3 contextsISessionFunctions(internal transactions)ISessionFunctionsTesting & Benchmarking: