Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(100)

Issue 10107004: Revert 132455 - Abort sync cycles when download step fails (Closed)

Created:
8 years, 8 months ago by rlarocque
Modified:
8 years, 8 months ago
Reviewers:
rlarocque
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 132455 - Abort sync cycles when download step fails This change causes the syncer to exit from its download then commit function if the download fails. This helps prevent the creation of server-side conflicts, which would be more likely to happen if a download failed but the following commit succeeded. The main changes are as follows: - Rather than proceed to the next step when a download updates failure is detected, the syncer will exit. This should cause the SyncScheduler to schedule a retry at a later time. - The definition of a download updates failure is now based on the return code of the download attempt, rather than checking the contents of the (possible non-existent) returned protobuf. This makes the error detection logic used here more closely match the logic used to decide whether or not to schedule retries. This implementation has a side-effect on configure sync cycles. The old behaviour was to handle failures by applying whatever updates we had downloaded at that point. The new behaviour will leave updates unapplied if any error occurs. This better matches a nearby comment's assertion which states that we should not attempt to apply updates until we have downloaded all of them. I believe the author of that comment would approve of this change. This change moves around some of the ExtensionActivityMonitor logic. It's important that we not take the data from the extensions acitivity monitor at the start of the cycle. Restoring that data is handled in the commit building and response code, which might not be executed if we need to break out early. This also fixes issue 123270. Most of the diffs for this change are concentrated in the unit tests: - Expose more of the SyncScheduler to the SyncerTest test harness. - Add functions to SyncerTest for testing specific types of sync jobs. - Add some functions that allow us to better control failures in MockConnectionManager. - Added tests for configure job success and failure. - Added tests for update then commit job success and failure. - Removed some redundant tests. BUG=122033, 123270 TEST=sync_unit_tests, specifically: SyncerTest.UpdateThenCommit, SyncerTest.UpdateFailsThenDontCommit, SyncerTest.ConfigureDownloadsTwoBatchesSuccess, SyncerTest.ConfigureFailsDontApplyUpdates Review URL: http://codereview.chromium.org/10006046 TBR=rlarocque@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132476

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -382 lines) Patch
M sync/engine/sync_scheduler.h View 3 chunks +6 lines, -7 lines 0 comments Download
M sync/engine/sync_scheduler.cc View 2 chunks +30 lines, -31 lines 0 comments Download
M sync/engine/syncer.cc View 3 chunks +17 lines, -25 lines 0 comments Download
M sync/engine/syncer_unittest.cc View 138 chunks +168 lines, -299 lines 0 comments Download
M sync/sessions/status_controller.h View 1 chunk +1 line, -2 lines 0 comments Download
M sync/sessions/sync_session_unittest.cc View 4 chunks +48 lines, -4 lines 0 comments Download
M sync/test/engine/mock_connection_manager.h View 3 chunks +3 lines, -10 lines 0 comments Download
M sync/test/engine/mock_connection_manager.cc View 2 chunks +3 lines, -4 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
rlarocque
8 years, 8 months ago (2012-04-16 22:19:00 UTC) #1

          

Powered by Google App Engine
This is Rietveld 408576698