Avoid useless SYNC_CYCLE_CONTINUATION sync cycle

Avoid useless SYNC_CYCLE_CONTINUATION sync cycle

When exiting from a sync cycle, the SyncScheduler will look at the results of
the previous cycle and use them to determine when the next sync job should run.
Part of this is detecting whether or not the job failed.

Right now, we assume that num_unsynced_handles > 0 implies that we failed to
commit everything we should have. That's wrong, because that information was
calculated at the beginning of the job and doesn't get updated following the
commit. So num_unsynced_handles > 0 actually implies that we probably tried to
commit something, but it doesn't necessarily mean the commit failed.

Because of that faulty logic, we assume that every sync cycle that commits
something has in fact failed. We schedule another job soon after to clean up
the failure.

This commit attempts to fix that issue. It moves the logic for detecting the
failure into SyncSession::Succeeded() which will act as an interpreter for the
session's state.  It also adds plumbing so that SyncerCommands can return
values indicating an error, and a some member variables in SyncSession to store
them.  This allows us to implement SyncSession::Succeeded() so it returns false
only when we know of an actual error, rather than trying to infer this from
unsynced_handles.

This commit also adds a similar function,
SyncSession::ExperiencedTransientError(), that will report based on the
returned error codes whether or not we should schedule another sync cycle or
give up.  There's not much point in retrying if the server is sending us
AuthErrors, and the return codes will allow us to see that information.
However, due to the risk of such a change, the ExperiencedTransientError()
function is not yet a part of the SyncScheduler's decision making.

Finally, this commit fixes issue 105994 because a recent change made it
so that I get -Werror compile errors locally because of that issue.

BUG=94670, 105994
TEST=

[rlarocque, 2011-12-23 02:30:19]

This mega-commit fixes SYNC_CYCLE_CONTINUATION GetUpdate bug.  It took many
patches to get to this point; now I can finally commit this without breaking
things.

This is a big change.  If you would prefer, I could split it up as follows:
- A patch to add return values to SyncerCommand
- A patch to process SyncerCommand results and implement
SyncSession::Succeeded()
- Finally, another patch to hook up SyncSession::Succeeded() to the
SyncScheduler.

Personally, I think it makes more sense this way.  If there's a bug in this
change, it's likely due to the way those patches interact with each other.  I'm
hoping that squashing this patch set will make those bugs easier to find.

The downside to this approach is that there are many kinds of errors we need to
watch for.  This change contains a (mostly side-effect free) refactoring of
SyncerCommand and one small change with big implications in SyncScheduler.

Reviewers, feel free to take your time.  I'm going on vacation for a week, and I
won't be looking to commit this patch until I get back.

[tim (not reviewing), 2012-01-04 20:30:21]

On 2011/12/23 02:30:19, rlarocque wrote:
> This mega-commit fixes SYNC_CYCLE_CONTINUATION GetUpdate bug.  It took many
> patches to get to this point; now I can finally commit this without breaking
> things.
> 
> This is a big change.  If you would prefer, I could split it up as follows:
> - A patch to add return values to SyncerCommand
> - A patch to process SyncerCommand results and implement
> SyncSession::Succeeded()
> - Finally, another patch to hook up SyncSession::Succeeded() to the
> SyncScheduler.
> 
> Personally, I think it makes more sense this way.  If there's a bug in this
> change, it's likely due to the way those patches interact with each other. 
I'm
> hoping that squashing this patch set will make those bugs easier to find.

I think seeing it together like this helps fill in the blanks that would be
there had you just sent out 3 patches as mentioned above because we can see how
things are supposed to interact.  But I still think landing it in 3 stages will
make everyone's (you, sheriffs, reviewers) lives easier for rebasing, landing on
trunk and dealing with fallout, allowing for more scrutiny of individual parts,
and in the future if anyone ever needs to revert a portion of it due to a latent
bug (and changes have landed atop this that use the new return value syntax, for
example).
> 
> The downside to this approach is that there are many kinds of errors we need
to
> watch for.  This change contains a (mostly side-effect free) refactoring of
> SyncerCommand and one small change with big implications in SyncScheduler.
> 
> Reviewers, feel free to take your time.  I'm going on vacation for a week, and
I
> won't be looking to commit this patch until I get back.

[rlarocque, 2012-01-04 22:50:22]

On 2012/01/04 20:30:21, timsteele wrote:
> On 2011/12/23 02:30:19, rlarocque wrote:
> > This mega-commit fixes SYNC_CYCLE_CONTINUATION GetUpdate bug.  It took many
> > patches to get to this point; now I can finally commit this without breaking
> > things.
> > 
> > This is a big change.  If you would prefer, I could split it up as follows:
> > - A patch to add return values to SyncerCommand
> > - A patch to process SyncerCommand results and implement
> > SyncSession::Succeeded()
> > - Finally, another patch to hook up SyncSession::Succeeded() to the
> > SyncScheduler.
> > 
> > Personally, I think it makes more sense this way.  If there's a bug in this
> > change, it's likely due to the way those patches interact with each other. 
> I'm
> > hoping that squashing this patch set will make those bugs easier to find.
> 
> I think seeing it together like this helps fill in the blanks that would be
> there had you just sent out 3 patches as mentioned above because we can see
how
> things are supposed to interact.  But I still think landing it in 3 stages
will
> make everyone's (you, sheriffs, reviewers) lives easier for rebasing, landing
on
> trunk and dealing with fallout, allowing for more scrutiny of individual
parts,
> and in the future if anyone ever needs to revert a portion of it due to a
latent
> bug (and changes have landed atop this that use the new return value syntax,
for
> example).

I have taken your advice and split this into four separate patches.  The first
one is the fix for issue 105994, and the following three are as described above.
 I will post these for review one at a time.  

Here is the first review: http://codereview.chromium.org/9087020/

Feel free to post comments to this review, or to the reviews for individual
patches.  I'll try to respond to them either way.