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

Finish postponed SCAN changes #501

Merged

Conversation

zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented May 15, 2024

Commit 07ed0ea introduced some SCAN improvements, but some changes were postponed to a later version (8.0), which this PR finishes:

  1. Prepare to move the TYPE filtering to the scan callback as well. this was put on hold since it has side effects that can be considered a breaking change, which is that we will not attempt to do lazy expire (delete) a key that was filtered by not matching the TYPE (changing it would mean TYPE filter starts behaving the same as MATCH filter already does in that respect).
  2. when the specified key TYPE filter is an unknown type, server will reply a error immediately instead of doing a full scan that comes back empty handed.

Fixes #235

Release notes:

SCAN: Expired keys that don't match the TYPE argument for the SCAN are no longer deleted by SCAN

Commit 07ed0ea introduced some SCAN improvements, but some changes were
postponed to a later version (8.0), which this PR finishes:

1. Prepare to move the TYPE filtering to the scan callback as well. this was
   put on hold since it has side effects that can be considered a breaking
   change, which is that we will not attempt to do lazy expire (delete) a key
   that was filtered by not matching the TYPE (changing it would mean TYPE filter
   starts behaving the same as MATCH filter already does in that respect).
2. when the specified key TYPE filter is an unknown type, server will reply a error
   immediately instead of doing a full scan that comes back empty handed.

Signed-off-by: Viktor Söderqvist <[email protected]>
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.80%. Comparing base (72f2a87) to head (91d30f1).
Report is 4 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #501      +/-   ##
============================================
+ Coverage     69.67%   69.80%   +0.12%     
============================================
  Files           109      109              
  Lines         61801    61797       -4     
============================================
+ Hits          43062    43137      +75     
+ Misses        18739    18660      -79     
Files Coverage Δ
src/db.c 88.15% <100.00%> (ø)

... and 11 files with indirect coverage changes

@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label May 15, 2024
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.

It's nice we wrote all these helpful comments in the code. We should do more of that.

@zuiderkwast
Copy link
Contributor Author

Yes, in this case the code was already written but commented out before merge. Should we list it as a breaking change?

@madolson
Copy link
Member

Yeah, as documented in the original issue:

1. Prepare to move the TYPE filtering to the scan callback as well. this was
  put on hold since it has side effects that can be considered a breaking
  change, which is that we will not attempt to do lazy expire (delete) a key
  that was filtered by not matching the TYPE (changing it would mean TYPE filter
  starts behaving the same as MATCH filter already does in that respect). 
2. when the specified key TYPE filter is an unknown type, server will reply a error
  immediately instead of doing a full scan that comes back empty handed. 

@zuiderkwast zuiderkwast added the breaking-change Indicates a possible backwards incompatible change label May 15, 2024
@zuiderkwast zuiderkwast merged commit efa8ba5 into valkey-io:unstable May 17, 2024
17 checks passed
@zuiderkwast zuiderkwast deleted the finish-postponed-scan-changes branch May 17, 2024 11:35
srgsanky pushed a commit to srgsanky/valkey that referenced this pull request May 19, 2024
Commit 07ed0ea introduced some SCAN improvements, but some
changes were postponed to a later version (8.0), which this PR finishes:

1. Prepare to move the TYPE filtering to the scan callback as well. this
was put on hold since it has side effects that can be considered a
breaking change, which is that we will not attempt to do lazy expire
(delete) a key that was filtered by not matching the TYPE (changing it
would mean TYPE filter starts behaving the same as MATCH filter already
does in that respect).
2. when the specified key TYPE filter is an unknown type, server will
reply a error immediately instead of doing a full scan that comes back
empty handed.

Fixes valkey-io#235

Release notes:

> SCAN: Expired keys that don't match the TYPE argument for the SCAN are
no longer deleted by SCAN

Signed-off-by: Viktor Söderqvist <[email protected]>
adetunjii pushed a commit to adetunjii/valkey that referenced this pull request May 22, 2024
Commit 07ed0ea introduced some SCAN improvements, but some
changes were postponed to a later version (8.0), which this PR finishes:

1. Prepare to move the TYPE filtering to the scan callback as well. this
was put on hold since it has side effects that can be considered a
breaking change, which is that we will not attempt to do lazy expire
(delete) a key that was filtered by not matching the TYPE (changing it
would mean TYPE filter starts behaving the same as MATCH filter already
does in that respect).
2. when the specified key TYPE filter is an unknown type, server will
reply a error immediately instead of doing a full scan that comes back
empty handed.

Fixes valkey-io#235

Release notes:

> SCAN: Expired keys that don't match the TYPE argument for the SCAN are
no longer deleted by SCAN

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Samuel Adetunji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a possible backwards incompatible change release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Finish implementing postponed SCAN changes
3 participants