search

contain-rs / lru-cache

A cache that holds a limited number of key-value pairs
https://contain-rs.github.io/lru-cache/lru_cache/
Apache License 2.0
82 stars 36 forks source link

make contains_key() read-only #48

Closed nlevitt closed 5 years ago

nlevitt commented 5 years ago

My expectation was that contains_key() would be read-only, not update the LRU state, and therefore not also require an exclusive lock to use.

If a user of the library wants to do the equivalent of contains_key() and update the LRU state, they can always do cache.get_mut(key).is_some(), which is pretty trivial code. On the other hand, without this change, there is no way to do a read-only contains_key().

FlashCat commented 5 years ago

Thanks for the pull request, and welcome! The contain-rs team is excited to review your changes, and you should hear from @cgaebel (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

JoshMcguigan commented 5 years ago

Currently the behavior is that calling contains_key for a given key updates the LRU state to mark that item as recently used and reduce that items chances of being removed from the cache. For that reason, this would be a breaking change.

It doesn't look like there is a unit test covering this behavior, but I'd be happy to create a PR to add a test here if that would be accepted by the contain-rs team.

Gankra commented 5 years ago

Yeah I agree it's a breaking change, even if perhaps an accidental feature (it's been too long to remember). Sorry.

nlevitt commented 5 years ago

:( how about new api contains_key_readonly() or something?

Gankra commented 5 years ago

I regret to inform you that:

WARNING: THIS PROJECT IS IN MAINTENANCE MODE, DUE TO INSUFFICIENT MAINTAINER RESOURCES

It works fine, but will generally no longer be improved.

We are currently only accepting changes which:

  • keep this compiling with the latest versions of Rust or its dependencies.
    • have minimal review requirements, such as documentation changes (so not totally new APIs).

The team that signed up to maintain these crates has dissolved, and I don't have the resources to things to the quality I expect. Also, I do not consider it safe to transfer control of these crates to strangers (as they may publish malicious or low-quality updates).

cgaebel commented 5 years ago

lmao

On Wed, Mar 6, 2019 at 9:08 PM Alexis Beingessner notifications@github.com wrote:

I regret to inform you that:

WARNING: THIS PROJECT IS IN MAINTENANCE MODE, DUE TO INSUFFICIENT MAINTAINER RESOURCES

It works fine, but will generally no longer be improved.

We are currently only accepting changes which:

  • keep this compiling with the latest versions of Rust or its dependencies.
  • have minimal review requirements, such as documentation changes (so not totally new APIs).

The team that signed up to maintain these crates has dissolved, and I don't have the resources to things to the quality I expect. Also, I do not consider it safe to transfer control of these crates to strangers (as they may publish malicious or low-quality updates).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/contain-rs/lru-cache/pull/48#issuecomment-470277162, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH3yaUscDnPEQid1QSSOoG4dePR2jUPks5vUC4-gaJpZM4Y6yUu .

-- Clark.

Key ID : 0x78099922 Fingerprint: B292 493C 51AE F3AB D016 DD04 E5E3 C36F 5534 F907