Closed Bug 388592 Opened 13 years ago Closed 13 years ago

respect the force=1 value on the query string for throttling

Categories

(AUS Graveyard :: General, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
4.x (triaged)

People

(Reporter: moco, Assigned: morgamic)

References

Details

Attachments

(1 file, 1 obsolete file)

respect the force=1 value on the query string for throttling

trunk (fx 3) and fx 2.0.0.5 are the first products to have bsmedberg's fix for bug #377088.

With that fix, if we enable throttling (see bug #388589 for example), we will still present updates to users who manually check for updates (but we will throttle background checks.)

this bug tracks the AUS side of the "force=1" feature.
Hey guys, looked at this already and I realized that passing update.xml?force=1 is different from what is used currently.

For /update/2/(.*)/update.xml, the incoming request routes $1 to ./index.php?path=$1 -- then AUS cleans and parses path.

The quickest way to do this is probably doing a strpos() to check for existence of force=1 in REQUEST_URI.  An alternative is using a RewriteCond on ${REQUEST_URI} but I'd rather use the PHP method.

Checking for force=1 is pretty straight forward -- I'll post a patch in a few minutes for review.
This checks for force=1 and doesn't allow throttling to continue if found.
Attachment #272812 - Flags: review?
Comment on attachment 272812 [details] [diff] [review]
v1, simple patch to check for force=1

Looking at this further, the strpos() call should be first in the if() -- so I will bump it up to the first check since it would short circuit everything else (skip it) when false.
Attachment #272812 - Flags: review? → review?(preed)
Duplicate of this bug: 384650
Duplicate of this bug: 388703
Comment on attachment 272812 [details] [diff] [review]
v1, simple patch to check for force=1

This looks good; as I mentioned on IRC, I'm curious what PHP's implementation of strpos() is, since searching the request URI for a string that (may or may not) be at the end seems like it would be expensive for every single hit.

It would be nice if we could let the webserver do the heavy lifting here and we could check for an environment variable, but you mentioned we rewrite the QUERY_STRING, so we can't do that (which I didn't know! :-)
Attachment #272812 - Flags: review?(preed) → review+
I think we should land this today.  Any objections?
Actually, I will test this (duh).  I can rerun AUS verification tests on aus2-staging vs. aus2.  For testing, I think the right strategy would be to set up:
1) throttling on aus2-staging to some arbitrary value (on, and maybe 50%)
2) keep aus2 productiona as-is of course
3) change aus2-staging test cases to include ?force=1
4) verify that even when throttling is turned on for aus2-staging, aus2-staging presents identical output to aus2 production.

Sound like a plan?

If this checks out we can push it Monday (it's getting later in the PM on a Friday and we agreed it's not the time to change AUS).
You could add a [QSA] flag to the RewriteRule line to have it append the new query string to the old one instead of replacing it.
(In reply to comment #9)
> You could add a [QSA] flag to the RewriteRule line to have it append the new
> query string to the old one instead of replacing it.
Yeah, this seems to work well.  I'd rather do it this way, so I'll repost a simple patch to the apache config.  Thanks Rob.
v2, based on comments from Rob.  [QSA] works fine and simplifies the check in index.php.  Will start tests shortly.
Attachment #272812 - Attachment is obsolete: true
Attachment #273512 - Flags: review?(preed)
Attachment #273512 - Attachment is patch: true
Attachment #273512 - Attachment mime type: application/octet-stream → text/plain
I checked this in to trunk and tagged it for staging so I could start testing. We can change it later and retest if you see any changes.  :)
This is up on aus2-staging if anybody wants to test it.  Examples:
https://aus2-staging.mozilla.org/update/2/Firefox/2.0/2006101023/WINNT_x86-msvc/en-US/release/update.xml?force=1

Versus:
https://aus2-staging.mozilla.org/update/2/Firefox/2.0/2006101023/WINNT_x86-msvc/en-US/release/update.xml

I'm starting the verification tests I used for the throttling now.
http://people.mozilla.com/~morgamic/aus2-tests/force-on.txt

Here are tests results from rerunning the throttle verification tests.  This tests that aus2-staging, when passed "?force=1", does not vary in any way from aus2 production.  All 15000+ tests passed as being identical.
Status: NEW → ASSIGNED
Comment on attachment 273512 [details] [diff] [review]
v2, same idea but uses query string append to pass force=1

I always screw up Apache rewrite rules, so you probably want someone else to r+ that part :-), but the rest looks good.
Attachment #273512 - Flags: review?(preed) → review+
Thanks Paul.  This was deployed 20 minutes ago.  Please reopen if you see any issues with AUS.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.