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 cache warmup strategy for OCS resource infos #1664

Merged
merged 4 commits into from
May 7, 2021

Conversation

ishank011
Copy link
Contributor

@ishank011 ishank011 commented Apr 26, 2021

Recently, a TTL cache was added to OCS to store statted resource infos #1643. This PR adds an interface to define warmup strategies and also adds a cbox specific strategy which starts a goroutine to initialize the cache with all the valid shares present in the system.

@refs @butonic @labkode please take a look at this when you get the time. I'm not 100% sure about the design so would love any feedback. I'm also thinking about adding a strategy with the json share + OCIS drivers.

@ishank011 ishank011 requested a review from labkode as a code owner April 26, 2021 14:22
@ishank011 ishank011 requested review from refs and butonic April 26, 2021 14:36
@butonic
Copy link
Contributor

butonic commented Apr 26, 2021

I don't feel fit for an in depth review today, but my mind immediately wandered towards caching the root etag for a storage space in the storage registry instead of the ocs or ocdav services. In any case, I think it makes sense to add a cache to the ocs service.

In my head a received share is also represented by a storage space. AFAICT the gateway and the storage providers have the ListStoragSpace API, which needs to still be implemented. So I am in favor of caching the results of the StorageSpace lists instead of adding a cach for stat results ... althoug one could argue that a storage space and a stat actually are the same thing.

I am trying to clarify the terminology. Maybe tomorrow a PR will be ready to clarify the terms ... just not feeling well ...

refs
refs previously approved these changes Apr 27, 2021
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a nitpick

@labkode labkode merged commit 71ce0a9 into cs3org:master May 7, 2021
@ishank011 ishank011 deleted the cache-warmup branch May 7, 2021 08:27
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.

4 participants