golang-lru icon indicating copy to clipboard operation
golang-lru copied to clipboard

added GetOrAdd to expirable LRU

Open dinghram opened this issue 1 year ago • 9 comments

dinghram avatar Aug 23 '23 19:08 dinghram

CLA assistant check
All committers have signed the CLA.

hashicorp-cla avatar Aug 23 '23 19:08 hashicorp-cla

+1, please merge.

aathan avatar Oct 23 '23 20:10 aathan

@paskal @aathan How do we get this merged? I do not have privilege to do so.

dinghram avatar Nov 20 '23 18:11 dinghram

@dinghram lru.Cache has a PeekOrAdd method very similar to your GetOrAdd method. Would changing your GetOrAdd method to be PeekOrAdd with the same signature still meet your needs? I would like to keep a consistent interface and add PeekOrAdd to the other LRUs in the package.

mgaffney avatar Jan 09 '24 19:01 mgaffney

@mgaffney Thanks for your response. From what I can tell, PeekOrAdd is a bit different. First, it does a "peek", which will not update the recentness of usage. Second, my GetOrAdd does an add-construct, where the passed in value is a function that will construct the new object if the object is not present. This is useful in my use case, where I am saving an object that has a sync.WaitGroup in it. The constructor-function I pass in creates the WaitGroup and adds 1 to it. The caller then waits for that group to be "done" if they didn't add the object. If they did add the object, they can fill out the cached object and then invoke "done" on the WaitGroup. This makes it possible to have many competing threads add a single object to the cache, and wait for one thread to fill it in, then they can all use it.

I'd be happy to change my method name to "GetOrAddConstruct" or something. However, as my method is not doing a Peek, but rather a Get, I don't think I should change the name to "Peek...". Do you agree?

dinghram avatar Jan 11 '24 21:01 dinghram

Hi @dinghram,

Yes, I can see that your GetOrAdd is very different from PeekOrAdd. Given that a constructor func might need to call a backend, can we change the signature to allow an error to be returned from both the constructor and the GetOrAdd method? Can we also rename the method to GetOrAddFunc since it is so different? Here's what I was thinking:

type ConstructorFunc[V any] func() (V, error)

func (c *LRU[K, V]) GetOrAddFunc(key K, fn ConstructorFunc[V]) (value V, added bool, evicted bool, err error) {

Thoughts?

mgaffney avatar Jan 16 '24 21:01 mgaffney

I agree. Changes are available now.

Thanks, Don Inghram

On Tue, Jan 16, 2024 at 2:09 PM Michael Gaffney @.***> wrote:

Hi @dinghram https://github.com/dinghram,

Yes, I can see that your GetOrAdd is very different from PeekOrAdd. Given that a constructor func might need to call a backend, can we change the signature to allow an error to be returned from both the constructor and the GetOrAdd method? Can we also rename the method to GetOrAddFunc since it is so different? Here's what I was thinking:

type ConstructorFunc[V any] func() (V, error) func (c *LRU[K, V]) GetOrAddFunc(key K, fn ConstructorFunc[V]) (value V, added bool, evicted bool, err error) {

Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/hashicorp/golang-lru/pull/155#issuecomment-1894516969, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFQJ6OMCV76AG56UMJSOFGTYO3UB3AVCNFSM6AAAAAA335ZBUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUGUYTMOJWHE . You are receiving this because you were mentioned.Message ID: @.***>

dinghram avatar Jan 19 '24 23:01 dinghram

@paskal @aathan @mgaffney Can this be merged?

dinghram avatar Mar 18 '24 19:03 dinghram

I have no opinion on this, as I am no longer working on the downstream project.

aathan avatar Mar 19 '24 16:03 aathan