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

store: refactor and improve store cache #14524

Merged

Conversation

bboozzoo
Copy link
Contributor

Refactor the store cache manager cleanup to make it easier to follow and do not keep repeating the same operations multiple times.

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

@bboozzoo bboozzoo added the Simple 😃 A small PR which can be reviewed quickly label Sep 20, 2024
Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

store/cache.go Outdated
return nil
}

var pruneCandidates []os.FileInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Just commenting in case it wasn't intentional but this no longer pre-allocates the slice. Probably not a big deal since this doesn't happen often or with lots of entries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. It was intentional, there should be few entries meeting the criteria. However, I'm thinking maybe it makes sense to preallocate the slice for say 1/4 or 1/5 length of entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a tweak for this. Please have a look

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

The change looks good.

Do we need a new test for this? I was expecting something not to be covered (hence the issue). Perhaps it's worth adding something?

@@ -165,54 +165,43 @@ func (cm *CacheManager) cleanup() error {
return err
}

// we need the modtime so convert to FileInfo
fil := make([]os.FileInfo, 0, len(entries))
if len(entries) <= cm.maxItems {
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart!

@bboozzoo bboozzoo force-pushed the bboozzoo/store-cache-improve branch from d1fab03 to 7cc600f Compare October 4, 2024 06:47
@bboozzoo
Copy link
Contributor Author

bboozzoo commented Oct 4, 2024

The change looks good.

Do we need a new test for this? I was expecting something not to be covered (hence the issue). Perhaps it's worth adding something?

There are tests covering this already. The branch is simply a small improvement I did while investigating another problem around the cache and pre-downloads.

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.97%. Comparing base (96ea7b0) to head (a268a30).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14524      +/-   ##
==========================================
+ Coverage   78.95%   78.97%   +0.02%     
==========================================
  Files        1084     1085       +1     
  Lines      146638   147115     +477     
==========================================
+ Hits       115773   116181     +408     
- Misses      23667    23713      +46     
- Partials     7198     7221      +23     
Flag Coverage Δ
unittests 78.97% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Refactor the store cache manager cleanup to make it easier to follow and
do not keep repeating the same operations multiple times.

Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo
Copy link
Contributor Author

bboozzoo commented Nov 1, 2024

Failing tests:

  • google:ubuntu-20.04-64:tests/main/nvidia-files:550_server - known issue with LP

@Meulengracht Meulengracht merged commit 6074d98 into canonical:master Nov 1, 2024
55 of 58 checks passed
sespiros added a commit to sespiros/snapd that referenced this pull request Nov 11, 2024
* master: (486 commits)
  o/snapstate, daemon: respect lanes that are passed to snapstate.InstallComponents, snapstate.InstallComponentPath (canonical#14695)
  cmd/snap-bootstrap: support for noexec and nodev options in preparation of tightening mount options for ubuntu core partitions
  o/hookstate: allow snapctl model command for kernel snaps
  many: complete various component cross-checks (canonical#14634)
  github: only check formatting when using latest/stable Go
  tests/main/auto-refresh-backoff: fix sorting of changes from the state (canonical#14691)
  store: place downloaded file in cache after rebuilding from delta (canonical#14680)
  interfaces/desktop: Improve launch entry and systray integration with session (canonical#14132)
  interfaces: desktop session improvements to shutdown,{system,upower}-observe and desktop  (canonical#13947)
  github: run make distcheck
  packaging: symlink fedora-latest to fedora packaging
  github: run unit tests on fedora:latest
  s/snapenv, snapdtool: fix tests that depend on user's SNAP_REEXEC (canonical#14681)
  store: refactor and improve store cache (canonical#14524)
  many: registry API creates transaction commit change (canonical#14675)
  tests/main/nvidia-files: set ubuntu-20.04-64/550-server to broken packaging in table of expectations (canonical#14679)
  c/snap-mgmt: unmount components before unmounting snaps (canonical#14664)
  .github/workflows/coverity-scan.yaml: add github action for Coverity (canonical#14654)
  data/selinux: remove timedatex (canonical#14670)
  c/snap-bootstrap: import users from hybrid system into recovery system (canonical#14521)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants