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

#182: Refactor command handling for maintainability #170

Merged
merged 5 commits into from
Jul 28, 2024

Conversation

kakdeykaushik
Copy link
Contributor

@kakdeykaushik kakdeykaushik commented Jul 19, 2024

#182

Dice cmds via struct

Summary of change:
Designed a struct to implement dice commands

Implementation details:
DiceCmdMeta encapsulates information about a dice command. A map map[string]DiceCmdMeta{} stores all the DiceCmdMeta objects and acts as a source of truth for all the commands.

  • Eval method contains business logic of the command.
  • if a command is not a valid DiceDB command then an error message is returned, earlier an invalid cmd was resolved as PING. This change is taken from Redis (refer img below)
  • few commands which requires Client struct are handled separately in func executeCommand as it was earlier. Handling them gracefully is a topic of discussion.
  • Info and Docs are kept placeholder and will be accomplished in Add support for COMMAND INFO command #148 and Add support for COMMAND DOCS command #146 respectively.

Redis reference:
Screenshot 2024-07-19 at 16 40 08

Note - changes in core/eval.go be better viewed in spit mode

core/commands.go Outdated Show resolved Hide resolved
core/commands.go Outdated Show resolved Hide resolved
Copy link
Contributor

@yashs360 yashs360 left a comment

Choose a reason for hiding this comment

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

Looking good, some minor changes requested.

@kakdeykaushik
Copy link
Contributor Author

kakdeykaushik commented Jul 26, 2024

@yashs360 implemented the changes you had mentioned. please review and lmk of any improvements. Tests(both unit and integration) are working correctly with the changes.

@gauravsarma1992
Copy link
Contributor

gauravsarma1992 commented Jul 27, 2024

@kakdeykaushik @yashs360 The Eval attribute of the DiceCmd struct should be an interface instead of a function type. I think the current attributes of the function type are not sufficient to satisfy all future use cases. Using interfaces as the function's input and output would help provide extensibility for future changes as well. One basic distinction I see in the current system is that some commands require only the args whereas some commands require the args along with the connection information. This will also allow us to define commands in different packages as well as long as the objects satisfy the interface requirements.

@JyotinderSingh
Copy link
Collaborator

JyotinderSingh commented Jul 28, 2024

@kakdeykaushik @yashs360 The Eval attribute of the DiceCmd struct should be an interface instead of a function type. I think the current attributes of the function type are not sufficient to satisfy all future use cases. Using interfaces as the function's input and output would help provide extensibility for future changes as well. One basic distinction I see in the current system is that some commands require only the args whereas some commands require the args along with the connection information. This will also allow us to define commands in different packages as well as long as the objects satisfy the interface requirements.

Thanks for the inputs and suggestions, Gaurav. Maybe we can do that as a separate effort when it starts to become a problem instead of prematurely making it generic.

@JyotinderSingh
Copy link
Collaborator

Thanks for the contribution @kakdeykaushik, and for the review @yashs360.

Merging this.

@JyotinderSingh JyotinderSingh changed the title ref: dice cmds into struct #182: Refactor command handling for maintainability Jul 28, 2024
@JyotinderSingh JyotinderSingh merged commit 8ca42c2 into DiceDB:master Jul 28, 2024
2 checks passed
gauravsarma1992 pushed a commit to gauravsarma1992/dice that referenced this pull request Aug 2, 2024
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