Add global cache for pushed times #612
Conversation
This might be a premature optimization, but after testing, I'm worried that listing statuses on busy repositories will end up being too expensive, even with the HTTP caching. Since these values are safe to cache forever, add a simple in-memory LRU cache in each server instance to minimize API calls. Also add a test for the PushedAt context function that verifies the caching behavior.
bluekeyes
force-pushed
the
bkeyes/cache-pushed-date
branch
from
d1dc5db
to
0605729
Compare
pull/github.go
Outdated
| ghc.pushedAt[sha] = pushedAt | ||
| if gc := ghc.globalCache; gc != nil { | ||
| gc.SetPushedAt(repoID, sha, pushedAt) |
There was a problem hiding this comment.
I need to fix this so that it doesn't cache the zero time, since that indicates "no pushed date" and is not globally cacheable.
Note that we might end up with the same commit having two pushed times as a result: the first evaluation will use the evaluation time and then future evaluations will use the time of the first status. I'd like to cache the evaluation time instead, but still trying to figure out how to do that in a way that makes sense, since it interacts with the commit filtering implemented at a different level.
|
Note for myself on how to fix the caching when there are no statuses:
This scanning will only happen when there are ignored commits (otherwise, we're always starting with the head commit), so should be pretty rare and the global cache will also help reduce the expense. This will also fix a bug when evaluating PRs that have a batch push ending in an ignored commit followed directly by another push containing only ignored commits. |
This required a bigger change than expected because we want to cache the evaluation timestamp when we use it. This meant moving the batch checking in to the PushedAt function instead of handling it in the approval logic. The new version is closer to how things used to work when we loaded the pushedDate field.
This might be a premature optimization, but after testing, I'm worried that listing statuses on busy repositories will end up being too expensive, even with the HTTP caching. Since these values are safe to cache forever, add a simple in-memory LRU cache in each server instance to minimize API calls.
Also add a test for the PushedAt context function that verifies the caching behavior.