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

Added support for COMMAND command #458

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

aashraybhandar1
Copy link
Contributor

Summary
Added support for COMMAND command. Please review.
COMMAND is used to return an array with details about every DiceDB command.
redis documentation : https://redis.io/docs/latest/commands/command/

Changes

  • Updated the existing evalCommand method to call the evalCommandDefault when no arguments are passed
  • Implemented evalCommandDefault which returns an array with details about every DiceDB command
  • made changes to resp.go to add appropriate formatting for []DiceCmdMeta and KeySpecs type
  • Added test and Benchmark

Benchmark results
Screenshot 2024-09-05 at 5 01 11 PM

Issue: #144

@JyotinderSingh
Copy link
Collaborator

Please rebase the PR, we just had a large change go in which might've introduced conflicts.

@aashraybhandar1
Copy link
Contributor Author

@JyotinderSingh due to this refatcoring, some changes of mine are breaking. The struct DiceCmdMeta is now not exposed to the package clientio which is needed to appropriately format the output for the COMMAND command. What should be the course of action? Will it be acceptable to move this struct to a new package called Models so that the cyclic dependency chain breaks?

internal/clientio/resp.go Outdated Show resolved Hide resolved
@arpitbbhayani
Copy link
Contributor

@aashraybhandar1 There are some test failures. Please fix the test failures and we will give it a final skim post that. thanks for the patch.

@aashraybhandar1
Copy link
Contributor Author

@lucifercr07 thanks for the suggestion. I have tried implementing it with this new commit. Would appreciate your feedback on the same. Thank You!

Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

Thanks @aashraybhandar1 for PR, please check the review comment once.

internal/eval/commands.go Outdated Show resolved Hide resolved
@lucifercr07
Copy link
Contributor

@aashraybhandar1 changes LGTM, will contain an issue to maintain the format parity with Redis.
Also we can update out command metadata to hold more info like flags and update arity for them.

Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

LGTM, Approved. Thanks @aashraybhandar1 for working on this.

@lucifercr07 lucifercr07 merged commit bb6df6f into DiceDB:master Sep 9, 2024
2 checks passed
rohitlohar45 pushed a commit to rohitlohar45/dice that referenced this pull request Sep 10, 2024
* Added support for COMMAND command
---------

Co-authored-by: Aashray Bhandari <aashraybhandari@Aashrays-MacBook-Air.local>
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.

4 participants