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

feat: add caching #150

Merged
merged 6 commits into from
Dec 10, 2024
Merged

feat: add caching #150

merged 6 commits into from
Dec 10, 2024

Conversation

d-loose
Copy link
Member

@d-loose d-loose commented Dec 4, 2024

Adds some simple caching logic to prevent too many unnecessary db requests, constant re-calculation of the top charts and spamming snapcraft.io with requests to resolve snap IDs.

  • charts (get_chart, cached for 24 hours)
  • snap_names (get_snap_name, cached indefinitely, currently used in the category updates, will also be used for the charts when migrating from IDs to names)
  • individual votes (get_by_snap_id, cached for 24 hours)

Personal vote data (get_all_by_client_hash) is not cached.

Caching can be disabled at compile time (for integration tests) via the SKIP_CACHE environment variable.

UDENG-5543

@d-loose d-loose marked this pull request as ready for review December 10, 2024 09:19
@d-loose d-loose requested review from matthew-hagemann and sminez and removed request for matthew-hagemann December 10, 2024 09:19
Comment on lines 54 to 59
if chart.was_cached {
info!(
"Using cached chart data for category '{:?}' in timeframe '{:?}'",
category, timeframe
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We said we'd look into the top level task periodically logging cache metrics at a later date but are we wanting to keep this info level logging each time a cached chart is served?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, this made it's way back in during the rebase - will remove it for now

sminez
sminez previously approved these changes Dec 10, 2024
Copy link
Collaborator

@sminez sminez left a comment

Choose a reason for hiding this comment

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

I'm assuming that the new trait bounds on Category, Timeframe and Chart are required by cached? Other than the comment around the logging this all looks good to go 👍

@d-loose
Copy link
Member Author

d-loose commented Dec 10, 2024

I'm assuming that the new trait bounds on Category, Timeframe and Chart are required by cached?

Yes, adding Hash allows us to directly use them as keys for the cache

@d-loose d-loose merged commit e304976 into main Dec 10, 2024
4 checks passed
@d-loose d-loose deleted the cache-chart-data branch December 10, 2024 12:05
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.

2 participants