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

Issue 11342008: Revert 164565 - sync: make scheduling logic and job ownership more obvious. (Closed)

Created:
8 years, 1 month ago by dharani
Modified:
8 years, 1 month ago
CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing)
Visibility:
Public.

Description

Revert 164565 - sync: make scheduling logic and job ownership more obvious. Reason for revert: check bug 158313 - inlines (and removes) SaveJob, to perform only the relevant "saving" bits where needed. As it turns out, most of it was only necessary for ShouldRunJob (which I'd like to rename, as it's destructive), and it's a lot easier to read what's happening and why. Note also removed TODO at SaveJob callsites. - inlines (and removes) InitOrCoalescePendingJob to perform only the relevant bits at each callsite. - pulls SyncSessionJob into a class, to handle basic responsibilities that need the job Purpose + session (like "Succeeded()" and "Finish") that were previously strewn about and partially copy/pasted in the scheduler. - meticulously transfers ownership (removes linked_ptr usage!) of jobs instead of making many implicit struct copies, as it resulted in non-obvious behavior. To do this, ownership is given to the scheduling mechanism (the MessageLoop for ScheduleSyncSessionJob, WaitInterval::timer for canary jobs), instead of jobs sitting around without knowing whether they're scheduled or not. There are a few cases where we can't actually "schedule" a job -- which wasn't even obvious from the old code, and other interesting revelations, like fact that we were actually pre-empt nudge canary jobs and "unscheduling" them in certain cases. - removes DoPendingJobIfPossible, since it is no longer needed for DoCanaryJob/Unthrottle cases, and make the behavior more explicit in the other cases (mode-switch and SCM error change), see TakePendingJobForCurrentMode. - removes the ability to create jobs as canary, since this wasn't needed and was adding complexity (see DoCanaryJob / GrantCanaryPrivilege). - removes retry_scheduled as it wasn't used anywhere - adds lots of comments. Also discovered that THROTTLED intervals seem to never fire a canary job with today's code (broken), config jobs are immune to per-data-type throttling, and the had_nudge allowance was defeated by extra IsBackingOff check in ScheduleNudgeImpl (see comment on review). BUG= Review URL: https://chromiumcodereview.appspot.com/10917234 TBR=tim@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164642

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+566 lines, -1091 lines) Patch
M sync/engine/sync_scheduler.h View 1 chunk +2 lines, -7 lines 0 comments Download
M sync/engine/sync_scheduler_impl.h View 8 chunks +74 lines, -50 lines 0 comments Download
M sync/engine/sync_scheduler_impl.cc View 27 chunks +341 lines, -356 lines 0 comments Download
M sync/engine/sync_scheduler_unittest.cc View 14 chunks +13 lines, -36 lines 0 comments Download
M sync/engine/sync_scheduler_whitebox_unittest.cc View 13 chunks +30 lines, -25 lines 0 comments Download
D sync/engine/sync_session_job.h View 1 chunk +0 lines, -124 lines 0 comments Download
D sync/engine/sync_session_job.cc View 1 chunk +0 lines, -184 lines 0 comments Download
D sync/engine/sync_session_job_unittest.cc View 1 chunk +0 lines, -239 lines 0 comments Download
M sync/engine/syncer.h View 1 chunk +1 line, -3 lines 0 comments Download
M sync/engine/syncer.cc View 2 chunks +7 lines, -6 lines 0 comments Download
M sync/engine/syncer_unittest.cc View 3 chunks +8 lines, -7 lines 0 comments Download
M sync/internal_api/js_sync_manager_observer_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M sync/internal_api/public/sessions/model_neutral_state.h View 1 chunk +0 lines, -2 lines 0 comments Download
M sync/internal_api/public/sessions/model_neutral_state.cc View 1 chunk +0 lines, -8 lines 0 comments Download
M sync/internal_api/public/sessions/sync_session_snapshot.h View 3 chunks +4 lines, -1 line 0 comments Download
M sync/internal_api/public/sessions/sync_session_snapshot.cc View 4 chunks +8 lines, -1 line 0 comments Download
M sync/internal_api/public/util/syncer_error.h View 1 chunk +0 lines, -2 lines 0 comments Download
M sync/sessions/session_state_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M sync/sessions/sync_session.h View 4 chunks +29 lines, -10 lines 0 comments Download
M sync/sessions/sync_session.cc View 5 chunks +42 lines, -16 lines 0 comments Download
M sync/sessions/sync_session_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M sync/sessions/test_util.cc View 3 chunks +1 line, -7 lines 0 comments Download
M sync/sync.gyp View 2 chunks +0 lines, -3 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
dharani
8 years, 1 month ago (2012-10-29 15:12:40 UTC) #1

          

Powered by Google App Engine
This is Rietveld 408576698