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)

defect
Not set
normal

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
Attached patch v1 (obsolete) — Splinter Review
* 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)
See Also: → 1257556
Attachment #8731698 - Flags: feedback?(rnewman)
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 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: nobody → mathieu
- Skip changes from other bucket
- Leverage ETag to limit bandwidth
- Use setting for collection name
Attachment #8731698 - Attachment is obsolete: true
Attachment #8736294 - Flags: review?(rnewman)
- Skip changes from other bucket
- Leverage ETag to limit bandwidth
- Use setting for collection name
Attachment #8736294 - Attachment is obsolete: true
Attachment #8736294 - Flags: review?(rnewman)
Attachment #8737232 - Flags: review?(rnewman)
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 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).
- Skip changes from other bucket
- Leverage ETag to limit bandwidth
- Use setting for collection name
Attached patch @rnewman review (obsolete) — Splinter Review
Attachment #8737774 - Attachment is obsolete: true
Attachment #8737775 - Flags: review?(rnewman)
Attached patch @rnewman review (obsolete) — Splinter Review
Attachment #8737775 - Attachment is obsolete: true
Attachment #8737775 - Flags: review?(rnewman)
Attachment #8737779 - Flags: review?(rnewman)
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)
Attachment #8737232 - Flags: review?(rnewman)
I'm confused by the two patches and I would like answers to comment 8.
See Also: → 1259145
> 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
- 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)
Attachment #8737232 - Attachment is obsolete: true
Attachment #8737779 - Attachment is obsolete: true
Attachment #8737779 - Flags: review?(MattN+bmo)
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?
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.
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
- 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)
Attachment #8738093 - Flags: review?(mgoodwin) → review+
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.
Attachment #8738935 - Flags: review?(mgoodwin) → review+
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 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 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+
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/
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/
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.
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 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+
Attachment #8741372 - Flags: review?(MattN+bmo) → review+
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 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+
Blocks: 1264914
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+
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.
https://hg.mozilla.org/mozilla-central/rev/b2c38c677cc7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.