Add option to make ProxyFetch forward incoming flushes#1336
Add option to make ProxyFetch forward incoming flushes#1336
Conversation
|
Hmm, how does the integration test work with the timer-based auto-flusher? It seems like there is a potential for flakes there. (Also I hope that works properly in nginx? Though I suppose it's not that effective when local end is producing a lot of data --- it was mostly meant for proxy use anyway) |
|
I did not test the timer-based auto-flusher, but I also think it's not enabled by default, so I wouldn't expect the test from this PR to be affected. To enable the timer-based flushing, you would need to configure as follows, right?: I can't find |
|
With regard to the timer-based flushing, I think |
| "wildcard_spec for referer urls which trigger blocking " | ||
| "rewrites"), | ||
| APACHE_CONFIG_OPTION(kModPagespeedFollowFlushes, | ||
| "Attempt to mirror incoming flushes for html streams in the output"), |
There was a problem hiding this comment.
Better to just give the option a name when registering this, then no code in the file is needed for mod_pagespeed to pick it up.
(Also it won't be inconsistent on whether it's directory vs. global or whatever.)
There was a problem hiding this comment.
... Though the description is likely to be confusing, since mod_pagespeed doesn't generally use ProxyFetch, and I am pretty sure InstawebContext does pass through all the Apache flushes unconditionally.
(Also this doesn't follow the style guide --- can't have one line with arguments indented and another at +4).
ngx_pagespeed currently buffers when rewriting HTML. This adds an option to make ProxyFetch follow up on any incoming Flush calls by requesting a Flush on it's associated RewriteDriver. This option is on by default, on parity with mod_pagespeed's behaviour. Includes a gtest for ensuring ProxyFetch complies and system tests to test for consistent default flushing behaviour accross modules. Accompanied by a PR in ngx_pagespeed's repo: apache/incubator-pagespeed-ngx#1217
oschaaf
force-pushed
the
oschaaf-master-flushing
branch
from
a834d30 to
1ddb563
Compare
|
@morlovich addressed your comments and force pushed, PTAL? |
| const char kModPagespeedForceCaching[] = "ModPagespeedForceCaching"; | ||
| const char kModPagespeedExperimentVariable[] = "ModPagespeedExperimentVariable"; | ||
| const char kModPagespeedExperimentSpec[] = "ModPagespeedExperimentSpec"; | ||
| const char kModPagespeedFollowFlushes[] = "ModPagespeedFollowFlushes"; |
|
Looks good, though I am not certain we need yet another flush knob. Shall I merge this in? |
|
@morlovich It would be great if you could merge it.. Will you take care of removing |
|
Yeah. (Will also need to emulate your PHP thing in google-specific tests, which may take a bit..) |
|
Hmm, this actually fails in Apache for me ---- the PHP flushes don't seem to produce APR_BUCKET_IS_FLUSH buckets for me.... |
|
Looks like it can be made to work with mod_Fastcgid handling PHP via: As hinted by this mod_fcgid changelog entry: Should be easy enough to add this into debug.conf.template guarded by an (Though I am having a different test fail afterwards, seemingly unrelated, but investigating) |
|
... And the "unrelated failure" was because some debug output I added was confusing the grep'ing of the debug log in that test. |
#1336 The tests are tweaked slightly to work with fcgid on Apache and some additional test environments.
|
Err, I think I pushed a merge of this, but somehow doesn't seem to show up in the Web gui. |
|
... And now that I complained it does. |
2 participants
ngx_pagespeed currently buffers when rewriting HTML. This adds an
option to make ProxyFetch follow up on any incoming Flush calls
by requesting a Flush on it's associated RewriteDriver. This option
is on by default, on parity with mod_pagespeed's behaviour.
Includes a gtest for ensuring ProxyFetch complies and system tests
to test for consistent default flushing behaviour accross modules.
Will be accompanied by a PR to ngx_pagespeed's repository.
Edit:
The nps pull is:
apache/incubator-pagespeed-ngx#1217