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

feat: Add slow flags for commands with heavy disk I/O #2494

Merged
merged 15 commits into from
Aug 19, 2024

Conversation

jonathanc-n
Copy link
Contributor

I have added slow flags for the following commands:

  • HGETALL
  • LINSERT
  • LREM
  • LRANGE
  • SCAN
  • SINTER
  • SDIFF
  • SUNION

I would like to just see if I had done these correctly, I am looking to add support for these commands as well (please verify if these seem to be commands that contribute to heavy disk I/O + lmk if there are more to add):

  • MSET
  • MGET
  • MSETNX
  • GETRANGE
  • SETRANGE
  • HKEYS
  • HVALS
  • HMGET
  • HRANGEBYLEX
  • LMPOP
  • BLMPOP
  • SCARD
  • SMEMBERS
  • ZRANGEBYSCORE
  • ZRANGEBYLEX
  • ZUNION
  • ZINTER
  • SORT

References: #2474

@jonathanc-n jonathanc-n changed the title Added slow flags for commands with heavy disk I/O feat: Add slow flags for commands with heavy disk I/O Aug 18, 2024
@git-hulk
Copy link
Member

git-hulk commented Aug 18, 2024

@jonathanc-n What this issue intends to add a command attribute like what we have in commands/commander.h#L66.

cc @PragmaTwice

And for the slow commands:

  • SCAN is not a slow command
  • HKEYS/HVALS are the same as HGETALL, and can be identified as an O(N) command
  • ZUNION/ZINTER are the same as SUNION
  • SMEMBERS is a slow command
  • *BYLEX commands behave like the SCAN command, so it shouldn't be a slow command

@jonathanc-n
Copy link
Contributor Author

@git-hulk Alright got it, thanks for the feedback!

@jonathanc-n
Copy link
Contributor Author

@git-hulk Are you able to check if this looks good? Thank you in advance!

src/commands/cmd_hash.cc Outdated Show resolved Hide resolved
src/commands/cmd_hash.cc Outdated Show resolved Hide resolved
src/commands/cmd_hash.cc Outdated Show resolved Hide resolved
src/commands/cmd_list.cc Outdated Show resolved Hide resolved
src/commands/cmd_server.cc Outdated Show resolved Hide resolved
src/commands/cmd_set.cc Outdated Show resolved Hide resolved
src/commands/cmd_zset.cc Outdated Show resolved Hide resolved
src/server/redis_connection.cc Outdated Show resolved Hide resolved
@jonathanc-n
Copy link
Contributor Author

@git-hulk Thanks for the review, sorry about the messy PR. I will make sure it is better in the future.

src/commands/cmd_list.cc Outdated Show resolved Hide resolved
src/commands/cmd_list.cc Outdated Show resolved Hide resolved
src/commands/cmd_list.cc Outdated Show resolved Hide resolved
src/commands/cmd_list.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@jonathanc-n A few new comments, rest are good to me. Thank you.

Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

I believe this is not enough, some command in kvrocks don't have a good implementation, so it's not slow in redis but slow in kvrocks

However, we can move forward firstly

Copy link

sonarcloud bot commented Aug 19, 2024

@git-hulk git-hulk merged commit f38557b into apache:unstable Aug 19, 2024
31 checks passed
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.

3 participants