Closed Bug 582083 Opened 11 years ago Closed 11 years ago

Should inspect POST responses for failed WBOs

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: philikon, Assigned: philikon)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
The PHP app is currently missing "modified". Its being fixed at #583096
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → philipp
Attachment #461545 - Flags: review?(edilee)
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.
Attachment #461545 - Flags: review?(edilee) → review+
(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. ;)
Attached patch v1.1Splinter Review
Adressed review comment (only loop once)
Attachment #461545 - Attachment is obsolete: true
1.4.x: http://hg.mozilla.org/services/fx-sync/rev/9891d99b99d1
default: http://hg.mozilla.org/services/fx-sync/rev/eaffcccd1744
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.