Merged
Conversation
Member
gwicke
commented
Mar 26, 2015
gwicke
commented
- https://phabricator.wikimedia.org/T93779 (only store if content differs)
- https://phabricator.wikimedia.org/T93777 (if-unmodified-since)
- Handle exceptions throwing falsy values better - Preserve full internal error objects & log them
We should allow modules to maintain their own metrics, so expose the metrics instance publicly rather than hiding it away in the _priv object.
Member
Author
This patch processes a no-cache update request for a parsoid content revision by letting Parsoid parse the page as usual. However, instead of blindly storing the new render, we now compare the content with the previous render's content & don't store a new revision if nothing changed. The hope is that this will reduce our storage requirements from template updates significantly, assuming that many template-triggered re-renders don't actually change the content. A statsd counter is set up to track the effectiveness of this optimization. Since Parsoid currently emits non-deterministic about attributes (see link to task), we need to perform some slightly hacky HTML normalization to avoid marking all HTML pages as differing. This hack should be removed once the Parsoid bug is fixed. I also used the opportunity to clean up a few things in the parsoid module: - Move the method setup to the mod constructor. This makes those methods available to the module itself without going through routing. Also removed the factory function in favor of simple currying / bind. - Create a new uuid directly in generateAndSave instead of passing it in from all users. The tests have been updated to use a simpler and thus more reliable set of pages to test changing vs. unchanging re-renders. A timestamp parser function is used to force updates in one of the pages. Bug: https://phabricator.wikimedia.org/T93779
gwicke
force-pushed
the
rerender
branch
2 times, most recently
from
2ca4854 to
02aa098
Compare
gwicke
changed the title
mods/parsoid.js
Outdated
Contributor
There was a problem hiding this comment.
If we want to stick to the spec, we should actually ignore If-Unmodified-Since when the date is invalid.
Member
Author
There was a problem hiding this comment.
Yeah, that's what the try/catch is about.
This patch cleans up the code paths in the parsoid module further by combining the revision fetching in getFormat with the one needed for the equality check as introduced in the previous patch. This also makes it easier to add if-unmodified-since support. On precondition failure, status 412 is returned and no parsing or other content is returned. Bug: https://phabricator.wikimedia.org/T93777
Contributor
There was a problem hiding this comment.
I agree with you on not exposing pagebundle, it's used only internally in this module anyway.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.