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

added GetOrAdd to expirable LRU #155

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

dinghram
Copy link

No description provided.

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 23, 2023

CLA assistant check
All committers have signed the CLA.

@aathan
Copy link

aathan commented Oct 23, 2023

+1, please merge.

@dinghram
Copy link
Author

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

@mgaffney
Copy link
Member

mgaffney commented Jan 9, 2024

@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.

@dinghram
Copy link
Author

@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?

@mgaffney
Copy link
Member

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?

@dinghram
Copy link
Author

dinghram commented Jan 19, 2024 via email

@dinghram
Copy link
Author

@paskal @aathan @mgaffney Can this be merged?

@aathan
Copy link

aathan commented Mar 19, 2024

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

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

Successfully merging this pull request may close these issues.

None yet

5 participants