Cache fixes and improvements #736
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… expiration * This ensures additions are directly detected for links whose cache has expired. * Expiration logged differently from deletions of obsolete URLs (debug level) * Note that the previous approach would have added the expired cache to the links to check, which is * generally inaccurate in terms of metadata update (cached metadata do not reflect the current sources) and corresponding reported failure locations. * wrong for the internal cache, where not considering the current metadata would prevent checking a newly-broken link for an existing cache key. This is addresses the points around metadata consistency and expiration raised when reviewing gjtorikian#712
* By extending an existing test to checking the size of metadata in the cache. * A duplicated entry in the test fixture, now fixed, also revealed the piling up of metadata. * This is what was seen e.g. in gjtorikian#733, with metadata for failures to be re-checked piling up across consecutive runs.
* This makes sure metadata for failures to be re-checked are not duplicated and do not pile up across consecutive runs, fixing what seen e.g. in gjtorikian#733. * We also explicitly ignore the URLs for which no metadata addition is detected. * This fixes the newly-extended test revealing the issues.
* The overall logic and approach is not changed, but should be easier to follow with focus on what matters, i.e. determining for which detected metadata a "matching" cached metadata exists and was found w/o failure. * No need to set anything in the metadata for what is found in the cache.
* To better reflect the logic and making it easy to follow in the code.
* This can greatly help tracking the actual behavior of the detection logic.
Done in commit 7dab503.
riccardoporreca
changed the title
* Closes gjtorikian#737, as a follow-up of splitting the timeframe in gjtorikian#708. * All cases are covered by tests.
gjtorikian
approved these changes
Jul 26, 2022
|
Looks good, thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While trying to look at the issue spotted in #733
I took the chance to review the current cache deletion / addition logic also following up on some of the review comments from #712 that affect the (mis)behaviour beyond performance considerations.
This also includes a small extension that would close #737.
I tried to be very detailed with individual commits and verbose messages describing the reason for each change, so this PR is best reviewed by looking at diffs and messages from individual commits.