582083 - Should inspect POST responses for failed WBOs

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

When uploading outgoing items, SyncEngine should inspect the server response of the POST (https://wiki.mozilla.org/Labs/Weave/Sync/1.0/API#POST) to see whether some WBOs failed to be updated on the server. Records that failed to be updated on the server should not be marked as synced.

Comment 1

[Tarek Ziadé (:tarek)]

Since this was not used before, I also suggest that we add in the 1.1 API in the structure we send back, an "invalid" entry containing a count of invalid wbos received in the batch. An invalid wbo is a wbo that was received without an id, or that couldn't be read (JSON error).  Then we can remove these entries from the "failures" list.

Comment 2

[Tarek Ziadé (:tarek)]

The PHP app is currently missing "modified". Its being fixed at #583096

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Make sure records that failed to upload continue to be marked in the tracker so that they'll be uploaded again in the next sync.

Comment 4

[Ed Lee :Mardak]

Comment on attachment 461545 [details] [diff] [review]
v1

>+++ b/services/sync/modules/engines.js
>+        for (let id in resp.obj.failed) {
>+          failed[id] = this._tracker.changedIDs[id];
>+        }
>+        let failed_ids = [id for (id in resp.obj.failed)];
>+        if (failed_ids.length)
>+          this._log.debug("Records that will be uploaded again because "
>+                          + "the server couldn't store them: "
>+                          + failed_ids.join(", "));
>+
Seems inefficient to loop over the same thing multiple times. Why not declare failed_ids before and push items inside the loop? The message could be put at the end if we want, but inside is fine too. Also note that messages are more for us than the end user as we're not localizing these strings.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> Seems inefficient to loop over the same thing multiple times.

Yeh, though it wouldn't at all surprise me if tracemonkey wouldn't brutally optimise that array comprehension. ;)

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1

Adressed review comment (only loop once)

Comment 7

[Philipp von Weitershausen [:philikon]]

1.4.x: http://hg.mozilla.org/services/fx-sync/rev/9891d99b99d1
default: http://hg.mozilla.org/services/fx-sync/rev/eaffcccd1744