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

FIELDEXPIRE / HEXPIRE #3772

Closed
wants to merge 3 commits into from
Closed

FIELDEXPIRE / HEXPIRE #3772

wants to merge 3 commits into from

Conversation

NegatioN
Copy link
Contributor

Hi :) Tagging @romange since I've only been speaking to you so far.

There are a few things missing in this PR, namely:

  1. The generic command FieldExpire is not yet implemented.
  2. There's still some mess to clean up.

I'd like some feedback on a few questions, and in general on the code:

  1. Is HEXPIRE supposed to call into the more general FieldExpire after modifying the CmdArgParser, or just use the same underlying common function for manipulating field ttl?
  2. Is there a way of returning an int-array in the Redis syntax to the client yet, or would I need to make that?
  3. Should I remove ReplaceObj and instead use AddOrFind since we always Find the fields before modifying their ttl?

I think I also need to re-arrange the code such that I ensure the correct encoding (which supports TTL) is only enforced once for each call, and then each field is operated on afterwards. But I can't see a whole lot of code sharing between sets and hsets here, other than that they're both operating on dense_sets. I'm a c++ noob though, so feel free to correct me on this.

This PR is related to: #3027

Thanks in advance.

@romange
Copy link
Collaborator

romange commented Sep 23, 2024

Thanks for submitting this PR. A few comments:

  1. This PR is quite big, so I suggest to divide it into several PRs where the first one covers only the data structures (under the core/ dir).
  2. Our core classes also have unit tests. I see you are focused on HEXPIRE, which means you will need string_map to support the TTL updates. I suggest adding unit tests to string_map_test.cc for that.
  3. Regarding AddOrFind - HEXPIRE never adds a field so I do not think AddOrFind is needed.

We can follow up on the questions related to the high level commands in the subsequent PRs.

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