valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Introduce HASH items expiration

Open ranshid opened this issue 7 months ago • 1 comments

State of this DRAFT

This is the initial draft to introduce support for volatile hash items. The complete design and plan is currently under review here and it meant to be used as a reference for the implementation suggested in this draft. The current implementation is NOT stable and require several refactors following comments and potential design changes.

This draft currently contains:

Functionality

  1. Ability to place expiration time as unix-time on Hash key items as well
  2. commands to set/get expiration time on Hash key items.
  3. Lazy expiration logic to expire items on access.

persistency

RDB and AOF modifications to support save/full sync replication/AOF rewrite/load

key-space-notifications

support was added for HASH operations as well as expiration events.

This Draft still lacks:

  1. SETEX/GETEX commands implementation
  2. Top comment needs to be refactored with the suggested design details
  3. signalKey should be presented
  4. Active Expiration logic
  5. statistics and observability

ranshid avatar May 15 '25 15:05 ranshid

Codecov Report

:x: Patch coverage is 76.09302% with 514 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 71.37%. Comparing base (dceb9f3) to head (4319edb). :warning: Report is 11 commits behind head on unstable.

Files with missing lines Patch % Lines
src/vset.c 51.27% 439 Missing :warning:
src/t_hash.c 94.88% 32 Missing :warning:
src/aof.c 26.08% 17 Missing :warning:
src/entry.c 95.40% 8 Missing :warning:
src/expire.c 95.27% 6 Missing :warning:
src/module.c 0.00% 5 Missing :warning:
src/rdb.c 85.18% 4 Missing :warning:
src/defrag.c 80.00% 3 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2089      +/-   ##
============================================
- Coverage     71.49%   71.37%   -0.13%     
============================================
  Files           123      125       +2     
  Lines         67487    69207    +1720     
============================================
+ Hits          48251    49395    +1144     
- Misses        19236    19812     +576     
Files with missing lines Coverage Δ
src/anet.c 72.44% <ø> (ø)
src/commands.def 100.00% <ø> (ø)
src/db.c 90.47% <100.00%> (+0.47%) :arrow_up:
src/hashtable.c 82.71% <100.00%> (+0.28%) :arrow_up:
src/lazyfree.c 86.39% <100.00%> (+0.28%) :arrow_up:
src/object.c 81.86% <100.00%> (+0.43%) :arrow_up:
src/server.c 88.40% <100.00%> (+0.33%) :arrow_up:
src/server.h 100.00% <ø> (ø)
src/t_string.c 96.34% <100.00%> (-0.50%) :arrow_down:
src/util.c 66.21% <100.00%> (+0.41%) :arrow_up:
... and 9 more

... and 12 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 18 '25 09:05 codecov[bot]

This is a lot of work you've done! I've only had time for a partial review today, but I had a few comments/questions so far. The command schema and entry memory layout looks good to me. It'll be interesting to see perf testing too! 😀

We are just more focused on introducing the functionality and would focus on performance testing as soon as possible.

ranshid avatar Jun 19 '25 05:06 ranshid

First comment - 37 changed files??? Dang!

JimB123 avatar Jun 24 '25 18:06 JimB123

First comment - 37 changed files??? Dang!

you still have another PR in the oven - it might be bigger than this :(

ranshid avatar Jun 25 '25 07:06 ranshid

Hi @ranshid , this is just the review of the entry.c code that you can check while I continue to review the rest of the code.

Thank you @rjd15372 !

TBH the entry is NOT the main focus of this PR. most of the entry code is taken from the already existing implementation of hashTypeEntry (with indeed some changes).

I think the really interesting part are the new commands themselves. this is were the complex logic is introduced (HSETEX, HGETEX, HEXPIRE etc...)

there is also the new volatile set API in the t_hash.c (that I do not like that much) but we can focus on this in the PR introducing the volatile set.

ranshid avatar Jul 01 '25 17:07 ranshid

Codecov Report

Attention: Patch coverage is 90.21739% with 90 lines in your changes missing coverage. Please review.

Project coverage is 71.59%. Comparing base (1e05724) to head (7a4eb30).

Files with missing lines Patch % Lines src/t_hash.c 94.20% 29 Missing ⚠️ src/aof.c 26.08% 17 Missing ⚠️ src/entry.c 90.18% 16 Missing ⚠️ src/volatile_set.c 66.66% 16 Missing ⚠️ src/expire.c 87.87% 4 Missing ⚠️ src/module.c 0.00% 4 Missing ⚠️ src/rdb.c 85.18% 4 Missing ⚠️ Additional details and impacted files 🚀 New features to boost your workflow:

I plan to extend the test suit by EOW

ranshid avatar Jul 01 '25 19:07 ranshid

there is a new memory leak which I have never seen before: https://github.com/valkey-io/valkey/actions/runs/16019103541/job/45191596412?pr=2089

Maybe related to the last refactoring. I will try and reproduce

ranshid avatar Jul 02 '25 08:07 ranshid

there is a new memory leak which I have never seen before: https://github.com/valkey-io/valkey/actions/runs/16019103541/job/45191596412?pr=2089

Maybe related to the last refactoring. I will try and reproduce

Fixed.

also added this test as part of the soon-to-be pushed suite:

test {HGETEX - verify no change when field does not exist} {
        r FLUSHALL
        r HSET myhash f1 v1
        set mem_before [r MEMORY USAGE myhash]
        assert_equal {{}} [r HGETEX myhash EX 1 FIELDS 1 f2]
        set memory_after [r MEMORY USAGE myhash]
        assert_equal $mem_before $memory_after
    }

ranshid avatar Jul 02 '25 11:07 ranshid

reviewers: note my last commit I verified it using the force defrag (which failed without this fix)

ranshid avatar Jul 02 '25 15:07 ranshid

reviewers, sorry. has to force push since I accidentally pushed changes from another branch (we will have to merge all the 3 PRs soon as working on3 branches is a nightmare)

ranshid avatar Jul 04 '25 05:07 ranshid

NOTE - we decided to remove the NX/XX options for HSETEX in order to be better aligned with existing clients. we will consider adding them later

ranshid avatar Jul 07 '25 16:07 ranshid

that will be great if we next time, when doing the rebase merge, we squash the PR number like #2089 in the commit message title. (I usually locate the PR web page based on the commit message title)

enjoy-binbin avatar Aug 06 '25 02:08 enjoy-binbin