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).

Details

Related Gerrit Patches:

Event Timeline

ori created this task.Apr 21 2016, 8:15 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 21 2016, 8:15 PM

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

aaron closed this task as Resolved.Apr 28 2016, 11:13 PM

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

ori added a comment.Apr 29 2016, 9:28 PM

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?