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 some instances of ExpiringCache not expiring cache items #3932

Merged
merged 2 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@erikjohnston
Member

erikjohnston commented Sep 21, 2018

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.

Fix some instances of ExpiringCache not expiring cache items
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 matrix-org/synapse-core Sep 21, 2018

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Sep 21, 2018

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

This comment has been minimized.

Member

richvdh commented Sep 25, 2018

we might want to consider adding a bit of jitter

this sounds like an orthogonal problem.

@richvdh

This comment has been minimized.

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

10 checks passed

ci/circleci: sytestpy2 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hawkowl hawkowl deleted the erikj/auto_start_expiring_caches branch Sep 25, 2018

richvdh added a commit that referenced this pull request Sep 26, 2018

Fix ExpiringCache.__len__ to be accurate
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

Remove redundant call to start_get_pdu_cache
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 join this conversation on GitHub. Already have an account? Sign in to comment