Skip to content
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

[Compatibility] Adding RENAMENX command #661

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Vijay-Nirmal
Copy link
Contributor

Adding support for RENAMENX command in Garnet

@Vijay-Nirmal
Copy link
Contributor Author

Is there a way to check the existence of the object without calling GET which the current EXISTS method does?
Or is there a similar method for object type like SET NX?

@TalZaccai TalZaccai self-requested a review September 18, 2024 18:12
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Great job adding such comprehensive testing.
Added a few comments...


var oldKeySlice = parseState.GetArgSliceByRef(0);
var newKeySlice = parseState.GetArgSliceByRef(1);
var status = storageApi.RENAMENX(oldKeySlice, newKeySlice);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GarnetStatus is not meant to be interpreted this way, you should pass an output parameter to this method and write the appropriate output to it.

@@ -620,6 +621,7 @@ private RespCommand FastParseCommand(out int count)
(2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("INCRBY\r\n"u8) => RespCommand.INCRBY,
(2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("DECRBY\r\n"u8) => RespCommand.DECRBY,
(2 << 4) | 6 when lastWord == MemoryMarshal.Read<ulong>("RENAME\r\n"u8) => RespCommand.RENAME,
(2 << 4) | 8 when lastWord == MemoryMarshal.Read<ulong>("NAMENX\r\n"u8) => RespCommand.RENAMENX,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to read the 'RE' prefix as well

/// GarnetStatus NOTFOUND means old key does not exists
/// GarnetStatus MOVED means new key already exists
/// </returns>
public unsafe GarnetStatus RENAMENX(ArgSlice oldKeySlice, ArgSlice newKeySlice, StoreType storeType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is majorly similar to RENAME, I would suggest adding a bool parameter to the RENAME method to specify if rename should happen only if key doesn't exist, instead of copying all this code (as any future change to this would need to happen in multiple locations).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants