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

fix: Use unique Cache files for symcaches with markers #982

Closed
wants to merge 1 commit into from

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Jan 19, 2023

This gives symcaches a unique cache key depending on the secondary_sources and the same cache file is not being reused with different sets of secondary_sources.

It also paves the way to avoid appending markers to files and thus getting rid of the should_load check in the future.

This still does not solve the edge-case when any of the secondary_sources itself changes. But that is the same edge-case we currently have if files on external symbol sources are overwritten. As long as we have that file cached, we will never try to re-download that file.
Cache files are considered to be immutable.

#skip-changelog

@Swatinem Swatinem marked this pull request as ready for review January 20, 2023 08:46
@Swatinem Swatinem requested a review from a team January 20, 2023 08:46
@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Base: 70.27% // Head: 70.27% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (c5cdd50) compared to base (5e12555).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
- Coverage   70.27%   70.27%   -0.01%     
==========================================
  Files          74       74              
  Lines       11606    11615       +9     
==========================================
+ Hits         8156     8162       +6     
- Misses       3450     3453       +3     
Impacted Files Coverage Δ
...olicator-service/src/services/symcaches/markers.rs 100.00% <100.00%> (ø)
...symbolicator-service/src/services/symcaches/mod.rs 91.01% <100.00%> (+0.10%) ⬆️
crates/symbolicator-service/src/services/cacher.rs 82.91% <0.00%> (-0.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Swatinem
Copy link
Member Author

We should be cautious when we roll this out, as this change here means that all the symcaches that are using some kind of secondary source won’t be found in the cache anymore.

Not sure if we should go forward with this right now, or rather make progress on #983, especially related to forward compatibility.

@Swatinem Swatinem marked this pull request as draft January 20, 2023 10:47
@ashwoods ashwoods closed this Feb 6, 2023
@Swatinem Swatinem deleted the fix/unique-marker branch February 20, 2023 10:41
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.

3 participants