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

Add 'WithDictIndex' expiry API and update RANDOMKEY command #1155

Merged
merged 8 commits into from
Oct 17, 2024

Conversation

nadav-levanoni
Copy link
Contributor

#1145

First part of a two-step effort to add WithSlot API for expiry. This PR is to fix a crash that occurs when a RANDOMKEY uses a different slot than the cached slot of a client during a multi-exec.

The next part will be to utilize the new API as an optimization to prevent duplicate work when calculating the slot for a key.

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.68%. Comparing base (0142198) to head (0739d6a).
Report is 16 commits behind head on unstable.

Files with missing lines Patch % Lines
src/db.c 97.36% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1155      +/-   ##
============================================
+ Coverage     70.66%   70.68%   +0.02%     
============================================
  Files           114      114              
  Lines         61716    61799      +83     
============================================
+ Hits          43611    43683      +72     
- Misses        18105    18116      +11     
Files with missing lines Coverage Δ
src/db.c 88.51% <97.36%> (+0.09%) ⬆️

... and 15 files with indirect coverage changes

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.

The code looks OK to me, I made a general comment about using dict_index and kvstore index, since is a bit more consistent within the file.

@zuiderkwast
Copy link
Contributor

@nadav-levanoni Please fix the DCO issue, as described in the Details link next to the failed DCO CI job (or here). What is DCO and why we need it is described in CONTRIBUTING.md.

@nadav-levanoni nadav-levanoni changed the title Add 'WithSlot' expiry API and update RANDOMKEY command Add 'WithDbIndex' expiry API and update RANDOMKEY command Oct 17, 2024
@madolson madolson changed the title Add 'WithDbIndex' expiry API and update RANDOMKEY command Add 'WithDictIndex' expiry API and update RANDOMKEY command Oct 17, 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.

Thanks!

@madolson madolson linked an issue Oct 17, 2024 that may be closed by this pull request
@madolson madolson added the release-notes This issue should get a line item in the release notes label Oct 17, 2024
@madolson madolson merged commit 136d0fd into valkey-io:unstable Oct 17, 2024
46 checks passed
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Oct 20, 2024
…o#1155)

valkey-io#1145

First part of a two-step effort to add `WithSlot` API for expiry. This
PR is to fix a crash that occurs when a RANDOMKEY uses a different slot
than the cached slot of a client during a multi-exec.

The next part will be to utilize the new API as an optimization to
prevent duplicate work when calculating the slot for a key.

---------

Signed-off-by: Nadav Levanoni <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Nadav Levanoni <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
madolson added a commit to madolson/valkey that referenced this pull request Jan 6, 2025
…o#1155)

valkey-io#1145

First part of a two-step effort to add `WithSlot` API for expiry. This
PR is to fix a crash that occurs when a RANDOMKEY uses a different slot
than the cached slot of a client during a multi-exec.

The next part will be to utilize the new API as an optimization to
prevent duplicate work when calculating the slot for a key.

---------

Signed-off-by: Nadav Levanoni <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Nadav Levanoni <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
madolson added a commit to madolson/valkey that referenced this pull request Jan 7, 2025
…o#1155)

valkey-io#1145

First part of a two-step effort to add `WithSlot` API for expiry. This
PR is to fix a crash that occurs when a RANDOMKEY uses a different slot
than the cached slot of a client during a multi-exec.

The next part will be to utilize the new API as an optimization to
prevent duplicate work when calculating the slot for a key.

---------

Signed-off-by: Nadav Levanoni <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Nadav Levanoni <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
madolson added a commit that referenced this pull request Jan 7, 2025
#1145

First part of a two-step effort to add `WithSlot` API for expiry. This
PR is to fix a crash that occurs when a RANDOMKEY uses a different slot
than the cached slot of a client during a multi-exec.

The next part will be to utilize the new API as an optimization to
prevent duplicate work when calculating the slot for a key.

---------

Signed-off-by: Nadav Levanoni <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Nadav Levanoni <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[CRASH] Cluster Mode Cross-Slot RANDOMKEY in MULTI-EXEC
4 participants