Page MenuHomePhabricator

Remove Parser::VERSION check in CacheTime
Closed, ResolvedPublic

Description

Currently, we record Parser::VERSION in CacheTime (the baseclass of ParserOutput), and compare it against Parser::VERSION again after loading from cache. We discard cache ventries with a mismatching parser version.

The idea behind this was to allow us to see the effect of changes to the parser immediately, ignoring stale entries in ParserCache. In practice, a cold ParserCache will immediately overload the application servers. For this reason, we have not bumped Parser::VERSION in a decade, even when substantial changes where made to the output.

For this reason, we should just drop the check. And we can probably just delete Parser::VERSION altogether.

The should be minimal; the only complication is that the check in CacheTime should be dropped at least one deployment cycle before removing the field or the constant. That way, we avoid trouble in case we have to roll back to code that performs the check on data written by code that didn't record the version.

Event Timeline

Change 649478 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] CacheTime: Remove the version check.

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

Change 649479 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] CacheTime: remove mVersion field

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

Change 649478 merged by jenkins-bot:
[mediawiki/core@master] CacheTime: Remove the version check.

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

How does Parsoid's HTML content version play into this? We've bumped Parsoid's HTML version in the past and anticipate doing that in the future.

How does Parsoid's HTML content version play into this? We've bumped Parsoid's HTML version in the past and anticipate doing that in the future.

We will add it in a different form when implementing content negotiation and accept headers for Parsoid. In the current form it used to unconditionally invalidate the whole ParserCache when bumped which would have disastrous consequences in production. Better just to remove it now instead of fiddling with it - with ParserCache it's safer to re-add a field when we need it then trying to repurpose an existing field.

Change 649479 merged by jenkins-bot:
[mediawiki/core@master] CacheTime: remove mVersion field

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

Pchelolo claimed this task.

Resolved long time ago