#145 : Added support for COMMAND COUNT command

DiceDB / dice

DiceDB is hyper-optimized for building and scaling truly real-time applications on modern hardware. It is a drop-in replacement for Redis with support for SQL-based reactivity.

https://dicedb.io/

Other

3.32k stars 420 forks source link

#145 : Added support for COMMAND COUNT command #153

Closed VipinRaiP closed 1 month ago

VipinRaiP commented 1 month ago

Summary

Added support for count command. Please review. COMMAND COUNT is used to get total number of commands in dicedb. redis documentation : https://redis.io/docs/latest/commands/command-count This is my first PR and just getting started with the project. I have referred @yashs360's PR #150 for making the code changes. Thank you @yashs360

Changes

Reused used the changes made by @yashs360 in PR #150 for adding constants for command names. Thanks @yashs360.
Updated the eval methods as per PR #158 to support COMMAND|subcommand, thanks @kakdeykaushik
Added method to get the total number of commands.
Added test and benchmark.

Benchmark results

Issue : #145

arpitbbhayani commented 1 month ago

@VipinRaiP in case you missed it. Let me know by where are you planning to wrap it up?

VipinRaiP commented 1 month ago

@VipinRaiP in case you missed it. Let me know by where are you planning to wrap it up?

Hi @arpitbbhayani Sorry for the delay, i will fix these issues and push updated code over this weekend . Please excuse

VipinRaiP commented 1 month ago

@VipinRaiP in case you missed it. Let me know by where are you planning to wrap it up?

I have fixed the issues and pushed a new commit, thanks

JyotinderSingh commented 1 month ago

Left some minor comments.

gauravsarma1992 commented 1 month ago

This PR https://github.com/DiceDB/dice/pull/170 will have some conflicts with the current PR.

VipinRaiP commented 1 month ago

This PR #170 will have some conflicts with the current PR.

Should this PR wait until #170 is merged and make changes as per new code?

JyotinderSingh commented 1 month ago

This PR #170 will have some conflicts with the current PR.

Should this PR wait until #170 is merged and make changes as per new code?

Hi @VipinRaiP, thanks for all the changes so far. #170 has been merged now. Please rebase your changes on top of the latest master and fix any conflicts that may be there. Once you have addressed the reviews, please resolve those comments and I will review this again.

VipinRaiP commented 1 month ago

This PR #170 will have some conflicts with the current PR.

Should this PR wait until #170 is merged and make changes as per new code?

Hi @VipinRaiP, thanks for all the changes so far. #170 has been merged now. Please rebase your changes on top of the latest master and fix any conflicts that may be there. Once you have addressed the reviews, please resolve those comments and I will review this again.

Rebased and pushed a new commit, please review

JyotinderSingh commented 1 month ago

Approved, will merge soon. Could you please update the benchmark screenshot (since we have made some changes now) @VipinRaiP

JyotinderSingh commented 1 month ago

@VipinRaiP Looks like a unit test is failing with these changes, could you please look into it?