Closed
Bug 1257533
Opened 8 years ago
Closed 8 years ago
Optimize and add safety checks in Kinto updater
Categories
(Cloud Services :: Firefox: Common, defect)
Cloud Services
Firefox: Common
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: leplatrem, Assigned: leplatrem)
References
Details
Attachments
(6 files, 6 obsolete files)
58 bytes,
text/x-review-board-request
|
MattN
:
review+
mgoodwin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MattN
:
review+
mgoodwin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
* Add a safety check to take advantage of buckets isolation on the server when polling for changes. In other words, ignore changes that do not match the bucket specified in configuration. * Save bandwidth when no changes occured on the server using ETag header and `304 Not modified` responses. * Fix inconstency where collection name was hard-coded in one place (`kinto-updater.js`) and in preference in other (`KintoCertificateBlock.js`)
Attachment #8731698 -
Flags: review?(mgoodwin)
Assignee | ||
Comment 1•8 years ago
|
||
Submitted job on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08219747211
Assignee | ||
Updated•8 years ago
|
Attachment #8731698 -
Flags: feedback?(rnewman)
Comment 2•8 years ago
|
||
Comment on attachment 8731698 [details] [diff] [review] v1 Review of attachment 8731698 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/kinto-updater.js @@ +16,2 @@ > const PREF_KINTO_LAST_UPDATE = "services.kinto.last_update_seconds"; > +const PREF_KINTO_LAST_TIMESTAMP = "services.kinto.last_timestamp"; This isn't a timestamp; it's an ETag. So s/timestamp/etag throughout. @@ +41,5 @@ > + // Use ETag to obtain a `304 Not modified` when no change occured. > + const lastTimestamp = Services.prefs.getCharPref(PREF_KINTO_LAST_TIMESTAMP, null); > + const headers = {}; > + if (lastTimestamp) { > + headers['If-None-Match'] = lastTimestamp; Double quotes not single quotes. @@ +54,5 @@ > Services.prefs.setIntPref(PREF_KINTO_CLOCK_SKEW_SECONDS, clockDifference); > > + // No changes since last time. > + if (response.status == 304) > + return; Braces on every conditional. @@ +62,5 @@ > let firstError; > for (let collectionInfo of versionInfo.data) { > + // Skip changes that don't concern configured blocklist bucket. > + if (collectionInfo.bucket != blocklistsBucket) > + continue; Same. @@ +85,5 @@ > // cause the promise to reject by throwing the first observed error > throw firstError; > } > + > + // Save current timestamp for next poll. etag throughout.
Attachment #8731698 -
Flags: feedback?(rnewman) → feedback+
Comment 3•8 years ago
|
||
Comment on attachment 8731698 [details] [diff] [review] v1 Review of attachment 8731698 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me with rnewman's feedback addressed; strictly the r+ should come from rnewman, not me.
Attachment #8731698 -
Flags: review?(mgoodwin) → feedback+
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mathieu
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcd61afca7f
Assignee | ||
Comment 5•8 years ago
|
||
- Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name
Assignee | ||
Updated•8 years ago
|
Attachment #8731698 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8736294 -
Flags: review?(rnewman)
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d7d1eec4d0
Assignee | ||
Comment 7•8 years ago
|
||
- Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name
Assignee | ||
Updated•8 years ago
|
Attachment #8736294 -
Attachment is obsolete: true
Attachment #8736294 -
Flags: review?(rnewman)
Assignee | ||
Updated•8 years ago
|
Attachment #8737232 -
Flags: review?(rnewman)
Updated•8 years ago
|
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Optimize and add safety checks in kinto updater → Optimize and add safety checks in Kinto updater
Target Milestone: mozilla48 → ---
Comment 8•8 years ago
|
||
Comment on attachment 8737232 [details] [diff] [review] Optimize and add safety checks in Kinto updater Review of attachment 8737232 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/kinto-updater.js @@ +41,2 @@ > > + // Use ETag to obtain a `304 Not modified` when no change occured. Nit: occured -> occurred @@ +91,5 @@ > throw firstError; > } > + > + // Save current Etag for next poll. > + const currentEtag = response.headers.get("ETag"); What do we do if this is missing? ::: services/common/tests/unit/test_kinto_updater.js @@ +61,4 @@ > // add a test kinto client that will respond to lastModified information > // for a collection called 'test-collection' > updater.addTestKintoClient("test-collection", { > + maybeSync: function(lastModified, serverTime){ Nit: space between ) and {. @@ +65,1 @@ > // ensire the lastModified and serverTime values are as expected While you're here: kill this misspelled and non-helpful comment line! @@ +85,5 @@ > && clockDifference >= Math.floor(startTime / 1000) - 2, true); > + > + // Last timestamp was saved. > + let lastEtag = Services.prefs.getCharPref(PREF_LAST_ETAG); > + do_check_eq(lastEtag, '"1100"'); Why does this value have quotes around it? @@ +111,5 @@ > const responses = { > "GET:/v1/buckets/monitor/collections/changes/records?": { > "sampleHeaders": [ > + "Content-Type: application/json; charset=UTF-8", > + "ETag: \"1100\"" OK, because this has quotes around it. Why does this etag have quotes around it? @@ +116,3 @@ > ], > "status": {status: 200, statusText: "OK"}, > + "responseBody": JSON.stringify({"data":[{ Spaces after colons (throughout).
Assignee | ||
Comment 9•8 years ago
|
||
- Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8737774 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8737775 -
Flags: review?(rnewman)
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8737775 -
Attachment is obsolete: true
Attachment #8737775 -
Flags: review?(rnewman)
Assignee | ||
Updated•8 years ago
|
Attachment #8737779 -
Flags: review?(rnewman)
Comment 12•8 years ago
|
||
Comment on attachment 8737779 [details] [diff] [review] @rnewman review I'd like to get MattN familiar with this chunk of code, 'cos I won't always be around to review it.
Attachment #8737779 -
Flags: review?(rnewman) → review?(MattN+bmo)
Updated•8 years ago
|
Attachment #8737232 -
Flags: review?(rnewman)
Comment 13•8 years ago
|
||
I'm confused by the two patches and I would like answers to comment 8.
Assignee | ||
Comment 14•8 years ago
|
||
> I'm confused by the two patches I am truely sorry, those are my first steps with contributing to FFx. My setup with MozReview is not ready yet unfortunately :( Meanwhile I will resubmit a merged of both patches. Answers to comment 8: > What do we do if this is missing? I added a safety check for the presence of ETag response header. The fix for bug 1259145 should also prevent this part of the code to be executed when the server response payload is not 200. > Why does this etag have quotes around it? An entity-tag consists of an opaque quoted string. [0][1] [0] https://tools.ietf.org/html/rfc7232#section-2.3 [1] http://kinto.readthedocs.org/en/latest/api/1.x/cliquet/timestamps.html
Assignee | ||
Comment 15•8 years ago
|
||
- Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name Review commit: https://reviewboard.mozilla.org/r/44301/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44301/
Attachment #8738093 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8737232 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8737779 -
Attachment is obsolete: true
Attachment #8737779 -
Flags: review?(MattN+bmo)
Comment 16•8 years ago
|
||
https://reviewboard.mozilla.org/r/44301/#review41225 ::: services/common/kinto-updater.js:67 (Diff revision 1) > + } > + > let versionInfo = yield response.json(); > > let firstError; > for (let collectionInfo of versionInfo.data) { Drive by (via bug 1259145) - we could check (and handle more gracefully) the condition where the response doesn't contain the data we expect?
Comment 17•8 years ago
|
||
Notice that we have a generic pattern for all calls, where we should check the status code we get back and parse the error. I don't know where this code should be (kinto.js vs Firefox layer) but it should be used for any call to kinto.
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8738093 [details] MozReview Request: Bug 1257533 - Add safety check when server is failing Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44301/diff/1-2/
Attachment #8738093 -
Attachment description: MozReview Request: Bug 1257533 - Optimize and add safety checks in Kinto updater → MozReview Request: Bug 1257533 - Add safety check when server is failing
Assignee | ||
Comment 19•8 years ago
|
||
- Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name Review commit: https://reviewboard.mozilla.org/r/44863/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44863/
Attachment #8738935 -
Flags: review?(mgoodwin)
Attachment #8738935 -
Flags: review?(MattN+bmo)
Attachment #8738093 -
Flags: review?(mgoodwin)
Updated•8 years ago
|
Attachment #8738093 -
Flags: review?(mgoodwin) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8738093 [details] MozReview Request: Bug 1257533 - Add safety check when server is failing https://reviewboard.mozilla.org/r/44301/#review42581 Looks good to me.
Updated•8 years ago
|
Attachment #8738935 -
Flags: review?(mgoodwin) → review+
Comment 21•8 years ago
|
||
Comment on attachment 8738935 [details] MozReview Request: Bug 1257533 - Optimize and add safety checks in Kinto updater https://reviewboard.mozilla.org/r/44863/#review42583
Comment 22•8 years ago
|
||
Comment on attachment 8738935 [details] MozReview Request: Bug 1257533 - Optimize and add safety checks in Kinto updater https://reviewboard.mozilla.org/r/44863/#review42885 ::: services/common/tests/unit/test_kinto_updater.js:64 (Diff revision 1) > + > + let syncPromise = new Promise(function(resolve, reject) { > // add a test kinto client that will respond to lastModified information > // for a collection called 'test-collection' > updater.addTestKintoClient("test-collection", { > - "maybeSync": function(lastModified, serverTime){ > + maybeSync: function(lastModified, serverTime) { Nit: you can go further an use newer method syntax with no `function` keyword ::: services/common/tests/unit/test_kinto_updater.js:133 (Diff revision 1) > + // if (req.headers.get("If-None-Match") == '"1100"') > + // "TypeError: req.headers.get is not a function" > + // https://developer.mozilla.org/en-US/docs/Web/API/Request/headers > + if (req._headers._headers["if-none-match"] && req._headers._headers["if-none-match"][0] == "\"1100\"") That documentation has nothing to do with the request object you have here: https://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/nsIHttpServer.idl#429 You want `getHeader(…)`.
Attachment #8738935 -
Flags: review?(MattN+bmo) → review+
Comment 23•8 years ago
|
||
Comment on attachment 8738093 [details] MozReview Request: Bug 1257533 - Add safety check when server is failing https://reviewboard.mozilla.org/r/44301/#review42887 ::: services/common/kinto-updater.js:57 (Diff revision 2) > + } > + else { Nit: cuddle the else ::: services/common/tests/unit/test_kinto_updater.js:110 (Diff revision 2) > + response.setStatusLine(null, 503, "Service Unavailable"); > + } missing `response.finish()` ::: services/common/tests/unit/test_kinto_updater.js:117 (Diff revision 2) > + } > + catch (e) { Nit: cuddle `catch`
Attachment #8738093 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 24•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46427/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46427/
Attachment #8741365 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8738935 [details] MozReview Request: Bug 1257533 - Optimize and add safety checks in Kinto updater Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44863/diff/1-2/
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8738093 [details] MozReview Request: Bug 1257533 - Add safety check when server is failing Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44301/diff/2-3/
Assignee | ||
Comment 27•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46429/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46429/
Attachment #8741372 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 28•8 years ago
|
||
https://reviewboard.mozilla.org/r/44301/#review42887 > missing `response.finish()` I realize that adding finish() make the test suite fails (looks like the response is returned with an invalid json body). In the docs it is said to be relevant only for async responses.
Assignee | ||
Comment 29•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46501/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46501/
Attachment #8741474 -
Flags: review?(MattN+bmo)
Comment 30•8 years ago
|
||
For mozreview you should be pushing commits as they will land. Therefore, when addressing review comments, the changes for the commit should be made to that commit then pushed back for review and mozreview will show an interdiff itself.
Comment 31•8 years ago
|
||
Comment on attachment 8741365 [details] MozReview Request: Bug 1257533 - Address MattN review https://reviewboard.mozilla.org/r/46427/#review43123 Please fold the follow-up commits into the original when landing.
Attachment #8741365 -
Flags: review?(MattN+bmo) → review+
Updated•8 years ago
|
Attachment #8741372 -
Flags: review?(MattN+bmo) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8741372 [details] MozReview Request: Bug 1257533 - Address others review comments,r=MattN https://reviewboard.mozilla.org/r/46429/#review43125 ::: services/common/tests/unit/test_kinto_updater.js:64 (Diff revision 1) > > let syncPromise = new Promise(function(resolve, reject) { > // add a test kinto client that will respond to lastModified information > // for a collection called 'test-collection' > updater.addTestKintoClient("test-collection", { > - maybeSync: function(lastModified, serverTime) { > + maybeSync: (lastModified, serverTime) => { I actually meant, not arrow functions: maybeSync(lastModified, serverTime) { … }
Comment 33•8 years ago
|
||
Comment on attachment 8741474 [details] MozReview Request: Bug 1257533 - Fix test suite,r=MattN https://reviewboard.mozilla.org/r/46501/#review43129
Attachment #8741474 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 34•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46773/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46773/
Attachment #8741815 -
Flags: review?(MattN+bmo)
Comment 35•8 years ago
|
||
Comment on attachment 8741815 [details] MozReview Request: Bug 1257533 - Address nit for fake sync client in tests,r=MattN https://reviewboard.mozilla.org/r/46773/#review43431 I'll fold all of these and land. In the future please squash follow-ups with the commit they're fixing before pushing for review.
Attachment #8741815 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 37•8 years ago
|
||
https://reviewboard.mozilla.org/r/46773/#review43431 Ok thanks. Although I use git interactive rebase several times a day, I don't know how to do that with Mercurial yet, but I'll ask for help.
Comment 38•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b2c38c677cc7
You need to log in
before you can comment on or make changes to this bug.
Description
•