[Sync] Fix sync scheduler/session logic determining successful commands.

[Sync] Fix sync scheduler/session logic determining successful commands.

We now have Succeeded(), which is true iff the command ran successfully, and
SuccessfullyReachedServer(), which is true iff we actually contacted the server
without errors.

BUG=131414
TEST=unit_tests


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

[Nicolas Zea, 2012-06-06 19:13:37]

PTAL, this is what we discussed yesterday.

[rlarocque, 2012-06-06 19:32:32]

IIRC, the Succeeded() flag was only used in two places:
- FinishSyncSessionJob(), which you modified.
- SyncSession::TakeSnapshot(), which eventually uses the flag to figure out
whether or not a job is currently queued up for retry in the SyncScheduler.

(That second use case is an unfortunate hack.)

We shouldn't need three flags for two use cases.  I suspect
'SuccessfullyReachServer()' is all we need; we could remove Succeeded() and the
finished_ flag.

https://chromiumcodereview.appspot.com/10537032/diff/1008/sync/sessions/sync_...
File sync/sessions/sync_session.h (right):

https://chromiumcodereview.appspot.com/10537032/diff/1008/sync/sessions/sync_...
sync/sessions/sync_session.h:135: bool SuccessfullyReachServer() const;
SuccessfullyReachedServer()?

[Nicolas Zea, 2012-06-06 20:36:15]

Discussed higher level comments offline. PTAL

https://chromiumcodereview.appspot.com/10537032/diff/1008/sync/sessions/sync_...
File sync/sessions/sync_session.h (right):

https://chromiumcodereview.appspot.com/10537032/diff/1008/sync/sessions/sync_...
sync/sessions/sync_session.h:135: bool SuccessfullyReachServer() const;
On 2012/06/06 19:32:32, rlarocque wrote:
> SuccessfullyReachedServer()?

Done.

[rlarocque, 2012-06-06 20:51:22]

One more comment.  I missed this on the first pass.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
File sync/sessions/sync_session.h (right):

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.h:226: bool finished_;
If the session requires multiple calls to SyncShare() (because of
HasMoreToSync()), we might end up reaching the "last step" more than once.

Maybe this should be set only when we leave the while loop in
SyncScheduler::DoSyncSessionJob()?

[rlarocque, 2012-06-06 21:12:05]

More comments.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
File sync/sessions/sync_session.cc (right):

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.cc:253: bool reach_server = false;
Rather than use an if statement, why not assign the result directly to the bool?
 

const bool reached_server =
  (error.last_download_updates_result == SYNCER_OK ||
   ...);

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.cc:258: return reach_server && !HadErrors(error);
This is trickier than it looks.  You got it right, but it probably deserves a
comment somewhere.  

We can't allow backoff relief based on the 'reach_server' condition alone.  That
would cause us to behave badly if the server was rejecting commits but accepting
GU requests.  

Could you add a comment explaining why it's necessary to check HadErrors here?

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
File sync/sessions/sync_session.h (right):

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.h:130: // Note: this method ignore Sync Protocol and
Client Actionable errors.
I don't think that's true.  Any response other than SYNCER_OK should be an
error.

Also, grammar nit: s/ignore/ignores/.

[Nicolas Zea, 2012-06-06 21:27:54]

PTAL. Also added a test for the case I missed/you caught.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
File sync/sessions/sync_session.cc (right):

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.cc:253: bool reach_server = false;
On 2012/06/06 21:12:05, rlarocque wrote:
> Rather than use an if statement, why not assign the result directly to the
bool?
>  
> 
> const bool reached_server =
>   (error.last_download_updates_result == SYNCER_OK ||
>    ...);

Done.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.cc:258: return reach_server && !HadErrors(error);
On 2012/06/06 21:12:05, rlarocque wrote:
> This is trickier than it looks.  You got it right, but it probably deserves a
> comment somewhere.  
> 
> We can't allow backoff relief based on the 'reach_server' condition alone. 
That
> would cause us to behave badly if the server was rejecting commits but
accepting
> GU requests.  
> 
> Could you add a comment explaining why it's necessary to check HadErrors here?

Done.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
File sync/sessions/sync_session.h (right):

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.h:130: // Note: this method ignore Sync Protocol and
Client Actionable errors.
On 2012/06/06 21:12:05, rlarocque wrote:
> I don't think that's true.  Any response other than SYNCER_OK should be an
> error.
> 
> Also, grammar nit: s/ignore/ignores/.

Protocol and actionable errors are not reflected in the SyncerError's for the
three commands, but in the protocol error and actionable error members of
ErrorCounter, which we're not checking here.

Protocol errors are handled by invoking OnSyncProtocolErrors, and actionable
errors are handled via OnActionableError.

I was tempted to check them as well, but if that were the case we would wind up
retrying on migration and other errors as well, which I believe you recently had
to fix.

https://chromiumcodereview.appspot.com/10537032/diff/6001/sync/sessions/sync_...
sync/sessions/sync_session.h:226: bool finished_;
On 2012/06/06 20:51:22, rlarocque wrote:
> If the session requires multiple calls to SyncShare() (because of
> HasMoreToSync()), we might end up reaching the "last step" more than once.
> 
> Maybe this should be set only when we leave the while loop in
> SyncScheduler::DoSyncSessionJob()?

Good catch.  I reset the flag in PrepareForAnotherSyncCycle now, as I'd still
rather ensure we completed the last cycle successfully.

[rlarocque, 2012-06-06 21:53:21]

Issue about the comment resolved offline.

LGTM once the comment is removed.

[commit-bot: I haz the power, 2012-06-06 21:54:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10537032/14002

[commit-bot: I haz the power, 2012-06-06 23:36:57]

Change committed as 140885