Add a five minute cache to get_destination_retry_timings #3933
Conversation
|
Hmm, this breaks metrics due to it returning a length estimate less than 0: |
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.
synapse/util/caches/expiringcache.py
Outdated
| if value is SENTINEL: | ||
| return default | ||
|
|
||
| if self.iterable: |
There was a problem hiding this comment.
I would not call this an eviction. To me, eviction means that we have removed a valid entry to make room for a new one. I'd consider this as equivalent to an update with a magic "doesn't exist" value.
There was a problem hiding this comment.
I think the problem is that we're potentially conflating "eviction because of invalidation" (which this code may do) with "eviction because of some automated process" in the statistics. If something pops from the cache so it won't be cached anymore deliberately, I don't think that's useful to track in what this metric appears to do (assessing whether our cache is effective).
There was a problem hiding this comment.
@hawkowl: so I think you're agreeing with me? If so I'll remove this bit of code and merge it.
There was a problem hiding this comment.
Ah, for some reason I thought that this was consistent with what we did elsewhere, but it isn't.
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)
synapse/util/caches/expiringcache.py
Outdated
| @@ -95,6 +98,13 @@ def __getitem__(self, key): | |||
|
|
|||
| return entry.value | |||
|
|
|||
| def pop(self, key, default=None): | |||
| value = self._cache.pop(key, SENTINEL) | |||
There was a problem hiding this comment.
this is the same as return self._cache.pop(key, default) now.
docstring wouldn't go amiss.
There was a problem hiding this comment.
this is the same as
return self._cache.pop(key, default)now.
It's not, actually, as self._cache.pop won't raise if default is None. Thinking about it we can probably just do return self._cache.pop(key, **kwargs)
There was a problem hiding this comment.
Argh, I've completely messed this up
| if value is SENTINEL: | ||
| raise KeyError(key) | ||
|
|
||
| return value |
There was a problem hiding this comment.
I could have just done:
def pop(self, key, *args, **kwargs):
"Identical functionality to `dict.pop(..)`"
return self._cache.pop(key, *args, **kwargs)But that feels quite opaque to me?
There was a problem hiding this comment.
yeah sorry, I hadn't quite twigged that dict.pop(foo) was different to dict.pop(foo, None).
Hopefully helps with #3931
Based on: #3932