Page MenuHomePhabricator

Dead code in ApiStashEdit::checkCache
Closed, ResolvedPublic

Description

The log data on fluorine and the metrics in graphite agree that quite a lot of the logic in ApiStashEdit::checkCache is never hit. We only ever seem to hit the cache_hit.presumed_fresh / cache_miss.no_stash cases. Lines 290 - 349 must not be getting hit at all (or is hitting some exception).

Event Timeline

Change 285766 had a related patch set uploaded (by Aaron Schulz):
Fix timestamp check in ApiStashEdit::checkCache

https://gerrit.wikimedia.org/r/285766

Change 285766 merged by jenkins-bot:
Fix timestamp check in ApiStashEdit::checkCache

https://gerrit.wikimedia.org/r/285766

Change 286097 had a related patch set uploaded (by Ori.livneh):
Fix timestamp check in ApiStashEdit::checkCache

https://gerrit.wikimedia.org/r/286097

Change 286097 merged by jenkins-bot:
Fix timestamp check in ApiStashEdit::checkCache

https://gerrit.wikimedia.org/r/286097

Deployment of the patch coincides with a ~60ms regression in page save time and a drop in the stash hit rate. This is not a surprise, since the patch dropped the freshness threshold from 5 minutes to 5 seconds. Given that the 5-minute threshold did not trigger complaints, should we consider bumping up the threshold to a higher value, perhaps as high as a full minute?