Skip to content

Conversation

@ranshid
Copy link
Member

@ranshid ranshid commented Jan 3, 2026

In the original implementation of Hash Field Expiration (#2089), the HSETEX command was implemented to report keyspace notifications only for performed changes. This is mostly aligned with other Hash commands (for example, HDEL will also not report hdel event for items which does not exist)
The HSETEX case is somewhat different and is more like the HSET case. During HSETEX, after the command validations pass, items are ALWAYS "added" to the object, even though they might not actually be added.
This case is the same for when the hash object is empty or when all the provided fields do not exist in the object (as reported here)

This PR changes the way HSETEX will report keyspace notifications so that:

  1. hset notification will ALWAYS be reported if all command validations pass.
  2. hexpire will be reported in case the command include an expiration time (even past time)
  3. hxpired will be reported in case the provided expiration time is in the past (or 0)
  4. hdel will be reported in case the hash exists (or created as part of the command) and following the command execution it was left empty.
  5. we will always return '1' as a return value of tHSETEX command which passed all validations. Before that we returned 1 only if we applied the change cross ALL the input fields, so in case some of them did not exist and a past time was set we would return 0.

@ranshid ranshid requested a review from madolson January 3, 2026 13:53
@ranshid ranshid added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Jan 3, 2026
@ranshid
Copy link
Member Author

ranshid commented Jan 3, 2026

currently making this a draft for when #2998 is merged

@codecov
Copy link

codecov bot commented Jan 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.86%. Comparing base (928e912) to head (33ef505).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3001      +/-   ##
============================================
- Coverage     74.93%   74.86%   -0.07%     
============================================
  Files           129      129              
  Lines         71145    71147       +2     
============================================
- Hits          53310    53264      -46     
- Misses        17835    17883      +48     
Files with missing lines Coverage Δ
src/t_hash.c 95.54% <100.00%> (+0.16%) ⬆️

... and 21 files with indirect coverage changes

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

@ranshid ranshid changed the title HSETEX - Aways issue keyspace notifications after validation HSETEX - Always issue keyspace notifications after validation Jan 4, 2026
@ranshid ranshid marked this pull request as ready for review January 4, 2026 14:49
@madolson
Copy link
Member

madolson commented Jan 5, 2026

So, I guess we either:

  1. Think this is a breaking change, and do it in 10.0
  2. We think this is a bugfix, and we backport it.

Do we have a preference between those two?

@ranshid
Copy link
Member Author

ranshid commented Jan 5, 2026

So, I guess we either:

  1. Think this is a breaking change, and do it in 10.0
  2. We think this is a bugfix, and we backport it.

Do we have a preference between those two?

TBH I am not sure how much this new feature adoption is big so I would love to make it ASAP.
Regarding the "bugfix" route - I guess this is fine by me and will save more confusion than a future "behavioural change"

@ranshid ranshid added major-decision-pending Major decision pending by TSC team and removed major-decision-pending Major decision pending by TSC team labels Jan 12, 2026
@ranshid ranshid requested a review from murphyjacob4 January 21, 2026 11:41
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

ranshid added a commit that referenced this pull request Jan 28, 2026
…3120)

The test is currently flakey, because until we merge:
#3001
when the expiration time provided is in the past, and the field does not
exist the HSETX will just silently ignore it, without incrementing the
statistics.

I prefer to focus on writing a dedicated test for:
#3001 and deflake this test now.

Signed-off-by: Ran Shidlansik <[email protected]>
ranshid added a commit to ranshid/valkey that referenced this pull request Jan 28, 2026
…alkey-io#3120)

The test is currently flakey, because until we merge:
valkey-io#3001
when the expiration time provided is in the past, and the field does not
exist the HSETX will just silently ignore it, without incrementing the
statistics.

I prefer to focus on writing a dedicated test for:
valkey-io#3001 and deflake this test now.

Signed-off-by: Ran Shidlansik <[email protected]>
ranshid added a commit to ranshid/valkey that referenced this pull request Jan 28, 2026
…alkey-io#3120)

The test is currently flakey, because until we merge:
valkey-io#3001
when the expiration time provided is in the past, and the field does not
exist the HSETX will just silently ignore it, without incrementing the
statistics.

I prefer to focus on writing a dedicated test for:
valkey-io#3001 and deflake this test now.

Signed-off-by: Ran Shidlansik <[email protected]>
ranshid added a commit to ranshid/valkey that referenced this pull request Jan 28, 2026
…alkey-io#3120)

The test is currently flakey, because until we merge:
valkey-io#3001
when the expiration time provided is in the past, and the field does not
exist the HSETX will just silently ignore it, without incrementing the
statistics.

I prefer to focus on writing a dedicated test for:
valkey-io#3001 and deflake this test now.

Signed-off-by: Ran Shidlansik <[email protected]>
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

I think this is right, and is worth backporting and calling out as a bug fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open.

Projects

Status: No status
Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants