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: implement resp INCR command #62

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

hderms
Copy link
Contributor

@hderms hderms commented Feb 7, 2023

Adds:

  • i64 parsing method that is similar to the Redis C implementation but is faster because of the use of checked_mul and checked_add available to Rust. It's slightly slower than the stdlib string::parse<i64>() at this point, but works from a &[u8] and respects Redis parsing rules, so is preferable for this use case
  • the RESP command parsing for INCR
  • the SET RESP command now eagerly checks whether the input value can be parsed as an i64, and if so, stores it as one. Otherwise it stores bytes (current behavior)
  • the implementation in Resp using seg entrystore

Notes:

  • segcache only handled uinsigned integers currently, so the implementation is a little strange. Essentially I am doing things on the Rust layer checked_add with the Segcache Value and determining whether it would overflow if we viewed the underlying u64 as an i64 and added to it. If it would not overflow, we simply update the underlying u64 using wrapping_add which would be guaranteed not to overflow provided nothing happens in between the check and set, which should be true currently. Adding i64 support to Segcache is probably desirable, but if storage is currently single-threaded, then the current implementation should work

src/storage/types/src/parse.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@brayniac brayniac left a comment

Choose a reason for hiding this comment

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

See feedback in-line.

@hderms hderms requested a review from brayniac February 8, 2023 19:46
src/entrystore/src/seg/resp.rs Fixed Show resolved Hide resolved
src/protocol/common/src/parsing.rs Fixed Show resolved Hide resolved
src/entrystore/src/seg/resp.rs Fixed Show resolved Hide resolved
@hderms
Copy link
Contributor Author

hderms commented Feb 21, 2023

@brayniac did you have any outstanding changes you wanted?

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.

2 participants