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 delete by func #44

Closed
mkalus opened this issue Feb 2, 2024 · 12 comments · Fixed by #45
Closed

Add delete by func #44

mkalus opened this issue Feb 2, 2024 · 12 comments · Fixed by #45
Labels
enhancement New feature or request

Comments

@mkalus
Copy link

mkalus commented Feb 2, 2024

I really like otter, but what do miss (in comparison to ristretto for example) is something similar to a tag option. I need it sometimes to invalidate a part of the cache, and an easy option (in my opinion at least) to implement this would be to add a function to delete a part of the the cache depending on a function.

This would go along something like this:

// DeleteByFunc removes the association for this key from the cache if the selectFunc returns true.
func (bs baseCache[K, V]) DeleteByFunc(selectFunc func(key K, value V) bool) {
	// aggregate all keys to delete
	keysToDelete := make([]K, 0)
	bs.Range(func(key K, value V) bool {
		if selectFunc(key, value) {
	            keysToDelete = append(keysToDelete, key)
        	}
	        return true
	})
	
	for _, key := range keysToDelete {
        	bs.Delete(key)
	}
}

You probably need a mutex, but the general idea should be clear.

Any additional ideas are appreciated.

@mkalus mkalus added the enhancement New feature or request label Feb 2, 2024
@maypok86
Copy link
Owner

maypok86 commented Feb 4, 2024

Hi, I think this can be added, but I'm wondering how the plan is to use this feature for your task.

I understand that you eventually want to do something like this.

m := map[int]bool{
    1: true,
    3: true,
    8: true,
}
c.DeleteByFunc(func(key int, value int) bool {
    return m[key]
})

Am I right? If so, I'm very curious how you will recognize m in advance.

Or will there be some kind of tag stored inside the keys or values and it can be checked in this function?

@maypok86
Copy link
Owner

maypok86 commented Feb 4, 2024

This could also be implemented more efficiently, but it probably doesn't make much sense given the very infrequent use of this operation.

@maypok86
Copy link
Owner

maypok86 commented Feb 4, 2024

Oh, it would be cool to get a link to a similar function in ristretto, because I couldn't find anything like that. :( This feature does look useful though.

@mkalus
Copy link
Author

mkalus commented Feb 5, 2024

I use GoCache on top of Ristretto. The use case is pretty specific - I cache read only objects in memory in order to speed up deserialization from DB. One nice feature of GoCache is the ability to invalidate multiple cached objects using tags, like described here:

https://github.com/eko/gocache?tab=readme-ov-file#cache-invalidation-using-tags

The actual code for Ristretto sits in https://github.com/eko/gocache/blob/master/store/ristretto/ristretto.go - the approach is simple enough (and a bit naive): Aggregate the cache keys and delete them one by one.


In the code above I was thinking about a similar possibility to simulate this. Mostly, I can traverse a cache object list and delete objects according to a specific pattern or criterium (like a field value). This would be pretty straightforward, although the whole thing has a few issues with concurrencies, but these are the same as in the implementation of GoCache.


An example to get the idea what I am thinking about:

type ExampleObject struct {
	Id             string
	ToDelete bool
}

//...

var exampleCache otter.Cache[string, ExampleObject] // cache key is Id of object

//...

exampleCache.DeleteByFunc(func(key string, value ExampleObject) bool {
	return value.ToDelete
})

Something along this line.

@maypok86
Copy link
Owner

maypok86 commented Feb 5, 2024

Hmm, that's still more like what the wrapper should implement.

It's not exactly what gocache does, its implementation could be much more efficient, but it does something very unpleasant in terms of allocations.

Ok, I'll add this feature, especially since it could close some other user requests as well, but if you really want something efficient in a case like this, it's better to do it on the wrapper side.

P.S. I have a suspicion that this solution would work even better than what gocache does, because the constant strings.Split and append look like a real pain.

maypok86 added a commit that referenced this issue Feb 5, 2024
@mkalus
Copy link
Author

mkalus commented Feb 6, 2024

P.S. I have a suspicion that this solution would work even better than what gocache does, because the constant strings.Split and append look like a real pain.

Thanks so much! And yes, I was quite shocked to see this yesterday 😸 Never had a closer look into the tag code yet.

maypok86 added a commit that referenced this issue Feb 6, 2024
@ben-manes
Copy link

Does your implementation delete the mapping even if it was changed by a concurrent write? If so you might make it conditional or document that behavior as a caveat. (Here is a Java version if that helps any)

@maypok86 maypok86 reopened this Feb 6, 2024
@maypok86
Copy link
Owner

maypok86 commented Feb 6, 2024

I decided to put it off for now (I shouldn't have 🙂), but it looks like a special effect of using RCU. Caffeine on the other hand is saved by using get + synchronized on updates.

Elsewhere otter uses node deletion to protect against this, so let's do the same here.

But the good news is that the Range implementation turned out to be smarter than I thought and it is finally possible to implement this function without keysToDelete.

@maypok86
Copy link
Owner

maypok86 commented Feb 6, 2024

Furiously wonders how Ben has time to review pull requests and issues in many repos...😁

@ben-manes
Copy link

I was super exhausted last night and went to bed early, then unfortunately woke up at 4am...

@maypok86
Copy link
Owner

maypok86 commented Feb 6, 2024

Ok, now the function implementation should be robust, if you know how to break its behavior now, then write.

@maypok86 maypok86 closed this as completed Feb 6, 2024
@mkalus
Copy link
Author

mkalus commented Feb 7, 2024

Looks cool - I will test it and report if I find any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants