Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix some instances of ExpiringCache not expiring cache items #3932

Merged
merged 2 commits into from
Sep 25, 2018

Conversation

erikjohnston
Copy link
Member

ExpiringCache required that start() be called before it would actually
start expiring entries. A number of places didn't do that.

This PR removes start from ExpiringCache, and automatically starts
backround reaping process on creation instead.

ExpiringCache required that `start()` be called before it would actually
start expiring entries. A number of places didn't do that.

This PR removes `start` from ExpiringCache, and automatically starts
backround reaping process on creation instead.
@erikjohnston erikjohnston requested a review from a team September 21, 2018 13:24
@erikjohnston
Copy link
Member Author

This seems to help quite a bit on jki.re, but we might want to consider adding a bit of jitter so that we don't end up with the entire cache being invalidated at once

@richvdh
Copy link
Member

richvdh commented Sep 25, 2018

we might want to consider adding a bit of jitter

this sounds like an orthogonal problem.

@richvdh
Copy link
Member

richvdh commented Sep 25, 2018

A number of places didn't do that.

I wonder which...

@richvdh richvdh merged commit 4c3e7ee into develop Sep 25, 2018
@hawkowl hawkowl deleted the erikj/auto_start_expiring_caches branch September 25, 2018 11:56
richvdh pushed a commit that referenced this pull request Sep 26, 2018
It used to try and produce an estimate, which was sometimes negative.
This caused metrics to be sad, so lets always just calculate it from
scratch.

(This appears to have been a longstanding bug, but one which has been made more
of a problem by #3932 and #3933).

(This was originally done by Erik as part of #3933. I'm cherry-picking it
because really it's a fix in its own right)
richvdh added a commit that referenced this pull request Sep 28, 2018
I think this got forgotten in #3932. We were getting away with it because it
was the last call in this function.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants