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.

Sign up for GitHub

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

Jump to bottom

Supported RESP Commands Info #287

Merged
merged 52 commits into from
May 9, 2024
Merged

Supported RESP Commands Info #287

merged 52 commits into from
May 9, 2024

Conversation

TalZaccai
Copy link
Contributor

  1. Restructuring RespCommandInfo to read supported RESP commands info from JSON file (Garnet.server/Resp/RespCommandsInfo.json)
  2. Adding support for specifying command info when registering custom commands (example implemented in GarnetServer, parsing custom command info from GarnetServer/CustomRespCommandsInfo.json)
  3. Implementing COMMAND, COMMAND INFO & COMMAND COUNT RESP commands, matching information returned with Redis
  4. Implementing serialization & deserialization of command info to/from JSON and to/from RESP
  5. Adding CommandInfoUpdater tool for easily adding / removing supported commands & sub-commands info, generating an updated JSON file to replace RespCommandsInfo.json
  6. Adding tests for new commands specified in #4

@TalZaccai TalZaccai added enhancement New feature or request API labels Apr 17, 2024
@TalZaccai TalZaccai requested review from badrishc and lmaas April 17, 2024 19:27
@TalZaccai TalZaccai linked an issue Apr 18, 2024 that may be closed by this pull request
COMMAND COUNT not supported #187
Closed
lmaas
lmaas reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@lmaas lmaas left a comment

Choose a reason for hiding this comment

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

Great work on this PR! I'm happy to see the command info becoming much more maintainable. From what I can see there are no major changes needed besides some additional cleanup (see below).

test/Garnet.test/RespCommandTests.cs Show resolved Hide resolved
test/Garnet.test/RespCommandTests.cs Show resolved Hide resolved
test/Garnet.test/RespScanCommandsTests.cs Outdated Show resolved Hide resolved
test/Garnet.test/TestUtils.cs Show resolved Hide resolved
playground/CommandInfoUpdater/SupportedCommand.cs Show resolved Hide resolved
libs/server/Resp/AdminCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/RespCommandInfoParser.cs Show resolved Hide resolved
libs/server/Resp/RespCommandInfoParser.cs Outdated Show resolved Hide resolved
libs/server/Resp/RespCommandInfoParser.cs Outdated Show resolved Hide resolved
libs/server/Resp/RespCommandsInfoProvider.cs Outdated Show resolved Hide resolved
@TalZaccai TalZaccai requested a review from lmaas May 3, 2024 03:36
lmaas
lmaas reviewed May 4, 2024
View reviewed changes
Copy link
Contributor

@lmaas lmaas left a comment

Choose a reason for hiding this comment

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

A few more small suggestions from my side, but I think the code looks good and can be merged afterwards.

libs/server/Resp/RespCommandInfoParser.cs Outdated Show resolved Hide resolved
playground/CommandInfoUpdater/SupportedCommand.cs Show resolved Hide resolved
test/Garnet.test/RespCommandTests.cs Show resolved Hide resolved
@TalZaccai TalZaccai requested a review from lmaas May 6, 2024 20:51
lmaas
lmaas suggested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@lmaas lmaas left a comment

Choose a reason for hiding this comment

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

Looks like this PR is still triggering a few warnings. Ideally we should try to get rid of all of them and suppress any warnings that are false positives.

I have added some suggestions below.

test/Garnet.test/RespCommandTests.cs Outdated Show resolved Hide resolved
playground/CommandInfoUpdater/CommandInfoUpdater.cs Show resolved Hide resolved
playground/CommandInfoUpdater/CommandInfoUpdater.cs Show resolved Hide resolved
playground/CommandInfoUpdater/CommandInfoUpdater.cs Show resolved Hide resolved
TalZaccai and others added 5 commits May 8, 2024 16:23
@TalZaccai @lmaas
Update test/Garnet.test/RespCommandTests.cs
c5c4659 
Co-authored-by: Lukas Maas <lumaas@microsoft.com>
@TalZaccai @lmaas
Update playground/CommandInfoUpdater/CommandInfoUpdater.cs
6dde1fd 
Co-authored-by: Lukas Maas <lumaas@microsoft.com>
@TalZaccai @lmaas
Update playground/CommandInfoUpdater/CommandInfoUpdater.cs
9405562 
Co-authored-by: Lukas Maas <lumaas@microsoft.com>
@TalZaccai @lmaas
Update playground/CommandInfoUpdater/CommandInfoUpdater.cs
1649db7 
Co-authored-by: Lukas Maas <lumaas@microsoft.com>
@TalZaccai
Disabling Nullable in CommandInfoUpdater.csproj + Fixing thread-safet…
56bcdaf 
…y issue in RespCommandsInfo initialization
@TalZaccai TalZaccai requested a review from lmaas May 8, 2024 22:45
lmaas
lmaas approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@lmaas lmaas left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the comments above. The code looks good to me now. I think we can go ahead and merge this into main. Thanks for your hard work! 🚀

@TalZaccai TalZaccai merged commit 7f1184d into main May 9, 2024
23 checks passed
@TalZaccai TalZaccai deleted the talzacc/respcommandinfo_refactor branch May 9, 2024 17:35
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PaulusParssinen PaulusParssinen PaulusParssinen left review comments

@lmaas lmaas lmaas approved these changes

@badrishc badrishc badrishc approved these changes

Assignees
No one assigned
Labels
API enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

COMMAND COUNT not supported
4 participants
@TalZaccai @lmaas @badrishc @PaulusParssinen