Introduce HASH items expiration
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
- Ability to place expiration time as unix-time on Hash key items as well
- commands to set/get expiration time on Hash key items.
- 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:
- SETEX/GETEX commands implementation
- Top comment needs to be refactored with the suggested design details
- signalKey should be presented
- Active Expiration logic
- statistics and observability
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 |
: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.
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.
First comment - 37 changed files??? Dang!
First comment - 37 changed files??? Dang!
you still have another PR in the oven - it might be bigger than this :(
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.
Codecov Report
Attention: Patch coverage is
90.21739%with90 linesin 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
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
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
}
reviewers: note my last commit I verified it using the force defrag (which failed without this fix)
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)
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
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)