karlseguin / ccache

A golang LRU Cache for high concurrency
MIT License
1.28k stars 119 forks source link

new: option to prevent calling onDelete when doing set on existing item #35

Closed primalmotion closed 4 years ago

primalmotion commented 4 years ago

This patch adds the option SkipDeleteCallbackOnSet that will prevent the cache for calling the onDelete callback when calling Set on an existing key.

karlseguin commented 4 years ago

The PR looks fine, but why?

I'm rusty with this library (and Go in general), but I enabling would result in the gc being the only way for the internal list to be cleaned up, right? What's the benefit there? The downside is that you'll purge more often and evict more live data (whereas if you delete as you go, you'll purge less often).

Thinking out load, maybe the benefit is to speed up sets, especially in the face of the deletables channel being full?

Since it's configurable, I think it's fine. But one of us should update the readme (and I'd like to give a use case / reasoning for why someone would set it).

karlseguin commented 4 years ago

Also curious what the case is considering you skip on Set but not Delete.

primalmotion commented 4 years ago

I did this for 2 reasons:

1) it looks very weird to me that we say "this item has been deleted" while it's more like "it has been updated" as you may want to treat this differently.

2) this is regarding the issue I opened (#34). The way we use the cache is the following:

So if we have in the cache the keys ["/a/b:xxx","/a:yyy", "/c:zzz"] we can receive an event that tells to invalidate all items related to /a which means we need to end up with ["/c:zzz"] so we can slowly rebuild the cache for /a and below.

As there is no way to range on the keys in the cache, we keep a small map of the keys we inserted, run the prefix check, and eventually delete the entries from the cache (yeah, it's messy, and I don't like it :-) ).

Since some requests can come in parallel we can end up with double set on the same key, causing onDelete to be called the second time, which makes the key from my secondary cache to be removed.

Now I realize that yes, this PR introduces issue regarding cleaning up the item and may not be really good. Looking at the code I could deal with this by sending a structure containing the item and the kind of operation (update or delete) here https://github.com/karlseguin/ccache/blob/9e7ee3fa29906fb993883bfed084fe0cac248398/cache.go#L197

But all of this could be removed and this PR closed if we can have DeletePrefix as you suggested in the issue. It would make my code way cleaner

primalmotion commented 4 years ago

But I still think calling onDelete for an update is weird :) so I can still implement this pr a bit more correctly

karlseguin commented 4 years ago

I'm happy with implementing a DeletePrefix (see other issue, your code review is appreciated since I've been spoiled with Elixir's concurrency simplicity for the last 4 years).

That said, I feel like this is the problem LayeredCaching was meant to solve.

So instead of having ["/a/b:xxx","/a:yyy", "/c:zzz"] as your keys, you'd have:

a
  b:xxx
  yyy
c
  zzz

via:

cache.Set("a", "b:xxx", value1)
cache.Set("a", "yyy", value2)
cache.Set("c", "zzz", value3)

which would then let you clear the a* permission via::

cache.DeleteAll("a")

The limitation of LayeredCache for your case (I think) would be if you need the flexibility of purging a:* as well as a:b* in which case the 2 levels of LayeredCache aren't enough. This is very likely something you need to support, but just making sure.

primalmotion commented 4 years ago

yeah I need to invalidate /a/b* so I would need a n-layeredCache. This is how I came to the conclusion that layered cache would not solve my issue

primalmotion commented 4 years ago

closing since implementing deletePrefixes is a more elegant and simpler way to handle this