-
Notifications
You must be signed in to change notification settings - Fork 434
feat: add value iterator #475
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
feat: add value iterator #475
Conversation
|
@mangalaman93 Please do let me know if this PR is sufficient for merged based on your earlier comment here #460 (review). I'm assuming based on the CLAssistant, @ilyatotl will need to sign the CLA, if not I might have to squash his commits. |
|
@SkArchon While I can appreciate the usefulness of a first class iterator, I'm worried about the increase in overall memory usage this PR will introduce. 16 bytes for the Could you not roll-your-own by embedding a Cache inside your own struct? Implement |
|
Hi @matthewmcneely, I am happy to remove the For context, we want to repopulate the ristretto cache upon restarts of our router (from the previous instance of the ristretto cache), so with evictions and such using the actual ristretto cache makes sense since it will be the source of truth. But on top of this we would also prefer to not do this if possible by having another map and lock on it for Set and Get wrappers since its on the hot path. |
|
@SkArchon Happy to review a version without the addition of an Item field. |
21be33a to
5112a10
Compare
|
@matthewmcneely I have updated the PR to have iterations only on values, please do let me know if there are any changes to be made, I also squashed the commit history due to the original author being inactive to accept the CLA. |
matthewmcneely
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, thanks!
|
Also do let me know if you would like me to mark the conversations as resolved, or to wait for you the reviewer to mark it as resolved. |
|
Hi @matthewmcneely, I see that you have resolved your previous review comments, please do let me know what else is needed to get this merged. |
|
@SkArchon Can you rebase your fork? I cannot resolve conflicts on forks. |
f52c09a to
fb3ddb3
Compare
|
Because of the merge commit I found it easier to squash. Anyway the merge-base is the tip of the main branch now. |
|
@matthewmcneely Quick question, would it be possible to have a ristretto release (so this would be included), I see the last release in August 2025. |
|
@SkArchon We released v2.4.0 yesterday. Thanks for your contributions. |
Description
This PR is based off of the PR here #460, I have opened a new PR as this only contains a subset of the changes, and I do not have access to push to the original authors fork.
This PR only contains the iteration function, which iterates on values.
cc: @ilyatotl, @mangalaman93
Checklist