Removed safe worker calculation from SyncScheduler.

Removed safe worker calculation from SyncScheduler.

Removed the calculation of restricted_workers from BuildModelSafeParams
and updated the SyncSession constructor to no longer require a workers
parameter. These changes are in line with bug 133030 and the change to
model changing commands so that they no longer post to unused workers.

BUG=133030
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157852

[rlarocque, 2012-09-19 20:15:33]

http://codereview.chromium.org/10947039/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

http://codereview.chromium.org/10947039/diff/1/sync/syncable/directory.cc#new...
sync/syncable/directory.cc:151: cryptographer_(cryptographer) {
This change doesn't belong in this patch.

[vishwath, 2012-09-19 20:31:17]

Undid the change to directory.cc. Noogler error with the git workflow, it
shouldn't happen again.

[rlarocque, 2012-09-19 20:39:21]

https://codereview.chromium.org/10947039/diff/1/sync/sessions/sync_session.cc
File sync/sessions/sync_session.cc (left):

https://codereview.chromium.org/10947039/diff/1/sync/sessions/sync_session.cc...
sync/sessions/sync_session.cc:78: workers_(workers),
What will be the effect of sync sessions with no workers?

I'm most concerned about model_changing_syncer_command.cc.

[vishwath, 2012-09-19 20:48:02]

Based on my understanding of the code, it seems like the only use for the
workers being passed in is to perform a check (for debug builds only) that
ensures that all enabled groups have an associated worker (lines 33-47 in
the old sync_session.cc). Since we don't need to worry about model changing
commands posting to unused workers anymore, it seems unnecessary to keep
it. Does that seem right to you?


On Wed, Sep 19, 2012 at 1:39 PM, <rlarocque@chromium.org> wrote:

>
> https://codereview.chromium.**org/10947039/diff/1/sync/**
>
sessions/sync_session.cc<https://codereview.chromium.org/10947039/diff/1/sync/sessions/sync_session.cc>
> File sync/sessions/sync_session.cc (left):
>
> https://codereview.chromium.**org/10947039/diff/1/sync/**
>
sessions/sync_session.cc#**oldcode78<https://codereview.chromium.org/10947039/diff/1/sync/sessions/sync_session.cc#oldcode78>
> sync/sessions/sync_session.cc:**78: workers_(workers),
> What will be the effect of sync sessions with no workers?
>
> I'm most concerned about model_changing_syncer_command.**cc.
>
>
https://codereview.chromium.**org/10947039/<https://codereview.chromium.org/1...
>

[rlarocque, 2012-09-19 20:59:02]

On 2012/09/19 20:48:02, vishwath wrote:
> Based on my understanding of the code, it seems like the only use for the
> workers being passed in is to perform a check (for debug builds only) that
> ensures that all enabled groups have an associated worker (lines 33-47 in
> the old sync_session.cc). Since we don't need to worry about model changing
> commands posting to unused workers anymore, it seems unnecessary to keep
> it. Does that seem right to you?
> 

It looks to me like the session's worker list will always be empty.  

If that's the case, then the loop in ModelChangingSyncerCommand::ExecuteImpl()
that posts tasks to the appropriate worker threads will never post any tasks,
and no work will be done.

Some parts of sync (ie. downloading updates) would still work, but others (ie.
applying them) would not.

[vishwath, 2012-09-20 00:02:19]

Changed patch to retain workers param in the SyncSession constructor, and
removed just the unneeded restricted_worker calculation in BuildModelSafeParams.

[rlarocque, 2012-09-20 00:41:24]

That looks much better.

https://codereview.chromium.org/10947039/diff/5002/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/10947039/diff/5002/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:318: active_groups.insert(group);
Can you remove active_groups?

[vishwath, 2012-09-20 16:32:24]

Removed active_groups from BuildModelSafeParams()

[rlarocque, 2012-09-20 17:15:35]

LGTM.  I've started a few trybots.

[commit-bot: I haz the power, 2012-09-20 19:08:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vishwath@google.com/10947039/12001

[commit-bot: I haz the power, 2012-09-20 21:53:36]

Change committed as 157852