sync: Follow-up to conflict resolution refactor

sync: Follow-up to conflict resolution refactor

This is part two of r164286.  That commit refactored the way we handle
conflict resolution.  This commit takes advantage of those changes to
delete lots of code.

Because this change deletes session_state.cc, I decided to move the two
remaining useful tests session_state_unittest.cc into their own files,
sync_session_snapshot_unittest.cc and sync_source_info_unittest.cc.  The
tests were not modified in any way.

None of these changes should have any effect on syncer behaviour:
- We can remove the simple conflict counters and related code, since we
  now assert that all conflicts will be resolved by the end of a
  successful sync cycle.
- We can remove the PerModelSafeGroupState because that struct no longer
  has any members.
- The 'conflicts_resolved' indicators are no longer set, so we can
  remove them.
- Without those indicators, it's no longer possible to have any
  SYNC_CYCLE_CONTINUATION sync cycles.  We can remove a few counters
  associated with them, as well as the 'has_more_to_sync' flag in the
  snapshot.
- Without SYNC_CYCLE_CONTINUATION cycles, there's no longer any need for
  code that loops around SyncShare() in SyncSchedulerImpl.  The
  SyncSession::PrepareForAnotherSyncCycle() function is no longer used,
  either.
- The ScopedConflictResolver installed on the session during a sync
  cycle is no longer used, so all the code related to it can be deleted.

BUG=147681, 111280, 156238
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165474

[rlarocque, 2012-10-26 18:35:21]

This is the follow-up to http://codereview.chromium.org/11192071/.  There should
be no logic changes in this one.  

Please review.

[Nicolas Zea, 2012-10-26 21:14:30]

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
File chrome/browser/sync/about_sync_util.cc (left):

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
chrome/browser/sync/about_sync_util.cc:250: IntSyncStat
simple_conflicts(section_this_cycle, "Simple Conflicts");
It's not clear to me we shouldn't track this anymore, although perhaps how we
track it should change. It seems to me it's useful to know how many conflicts we
hit this cycle (even if we resolve all of them).

http://codereview.chromium.org/11314008/diff/1/sync/engine/all_status.cc
File sync/engine/all_status.cc (left):

http://codereview.chromium.org/11314008/diff/1/sync/engine/all_status.cc#oldc...
sync/engine/all_status.cc:190: status_.nudge_source_continuation++;
What's the plan to do nudges for server conflicts? Should we still be getting
rid of this before we know how we want to handle those?

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
File sync/internal_api/public/sessions/sync_session_snapshot_unittest.cc
(right):

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
sync/internal_api/public/sessions/sync_session_snapshot_unittest.cc:5: #include
"sync/internal_api/public/sessions/sync_session_snapshot.h"
Can this test be placed in sync/sessions? Or at least sync/internal_api/?

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
File sync/internal_api/public/sessions/sync_source_info_unittest.cc (right):

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
sync/internal_api/public/sessions/sync_source_info_unittest.cc:5: #include
"sync/internal_api/public/sessions/sync_source_info.h"
Similar to the other test, does this have to be in internal_api/public?

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controller.h
File sync/sessions/status_controller.h (right):

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controlle...
sync/sessions/status_controller.h:10: // state, and can be called only when the
SafetyRestriction is in effect.  Other
I think the more indepth description that was here previously is still useful
right?

[rlarocque, 2012-10-26 21:41:46]

+Tim for his thoughts on the placment of the unittest.cc files.  Should they be
under public/ along with the files they test, or should they sit in the
equivalent non-public locations?

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
File chrome/browser/sync/about_sync_util.cc (left):

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
chrome/browser/sync/about_sync_util.cc:250: IntSyncStat
simple_conflicts(section_this_cycle, "Simple Conflicts");
On 2012/10/26 21:14:30, Nicolas Zea wrote:
> It's not clear to me we shouldn't track this anymore, although perhaps how we
> track it should change. It seems to me it's useful to know how many conflicts
we
> hit this cycle (even if we resolve all of them). 

Many of the conflict resolution scenarios are covered by other counters, such as
"Conflicts resolved: client wins" and "Conflicts resolved: server wins".  Does
that address your concern?

I suppose we won't track some of the other resolutions (changes match, ignore
encryption), but, to me, those don't seem to be worth tracking.

http://codereview.chromium.org/11314008/diff/1/sync/engine/all_status.cc
File sync/engine/all_status.cc (left):

http://codereview.chromium.org/11314008/diff/1/sync/engine/all_status.cc#oldc...
sync/engine/all_status.cc:190: status_.nudge_source_continuation++;
On 2012/10/26 21:14:30, Nicolas Zea wrote:
> What's the plan to do nudges for server conflicts? Should we still be getting
> rid of this before we know how we want to handle those?

I'm pretty sure this won't be part of the solution.  AFAIK, this value was never
used, aside from its definition and its appearance in a few case statements.

If we do end up using a nudge to resolve it, I'd prefer to call it
"NUDGE_SOURCE_CONFLICT_RETRY", or some other descriptive name.  More likely,
though, I think would be to implement a special backoff for the case where a
server detects conflicts.

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
File sync/internal_api/public/sessions/sync_session_snapshot_unittest.cc
(right):

http://codereview.chromium.org/11314008/diff/1/sync/internal_api/public/sessi...
sync/internal_api/public/sessions/sync_session_snapshot_unittest.cc:5: #include
"sync/internal_api/public/sessions/sync_session_snapshot.h"
On 2012/10/26 21:14:30, Nicolas Zea wrote:
> Can this test be placed in sync/sessions? Or at least sync/internal_api/?

That would have been my first choice, too.  But I found that it's common
practice to put the _unittest.cc files next to the functionality they test.  

$ cd src/sync/internal_api/public
$ ls */*unittest*
base/enum_set_unittest.cc
base/model_type_invalidation_map_unittest.cc
base/node_ordinal_unittest.cc
base/ordinal_unittest.cc
engine/model_safe_worker_unittest.cc
util/immutable_unittest.cc
util/weak_handle_unittest.cc

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controller.h
File sync/sessions/status_controller.h (right):

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controlle...
sync/sessions/status_controller.h:10: // state, and can be called only when the
SafetyRestriction is in effect.  Other
On 2012/10/26 21:14:30, Nicolas Zea wrote:
> I think the more indepth description that was here previously is still useful
> right?

The only remaining use of the ModelSafeGroupRestriction is
commit_id_projection().  The examples of conflict_progress() and commit_ids()
are no longer valid.  The comments about session_state.h are no longer valid,
since there is no longer any PerModelSafeGroupState.  Much of the "shared" state
are strictly increasing counters that are incremented on different threads, so
the comment about requiring a lack of group restriction no longer applies.

I'd be open to re-wording this comment if you think it leaves out some valuable
information or is less clear than the original.  However, much of the original
is out of date and had to be modified, if not deleted.

[Nicolas Zea, 2012-10-26 22:03:26]

LGTM with updated comment and once where to place the tests is resolved.

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
File chrome/browser/sync/about_sync_util.cc (left):

http://codereview.chromium.org/11314008/diff/1/chrome/browser/sync/about_sync...
chrome/browser/sync/about_sync_util.cc:250: IntSyncStat
simple_conflicts(section_this_cycle, "Simple Conflicts");
On 2012/10/26 21:41:46, rlarocque wrote:
> On 2012/10/26 21:14:30, Nicolas Zea wrote:
> > It's not clear to me we shouldn't track this anymore, although perhaps how
we
> > track it should change. It seems to me it's useful to know how many
conflicts
> we
> > hit this cycle (even if we resolve all of them). 
> 
> Many of the conflict resolution scenarios are covered by other counters, such
as
> "Conflicts resolved: client wins" and "Conflicts resolved: server wins".  Does
> that address your concern?
> 
> I suppose we won't track some of the other resolutions (changes match, ignore
> encryption), but, to me, those don't seem to be worth tracking.

Fair enough, I guess those should be sufficient.

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controller.h
File sync/sessions/status_controller.h (right):

http://codereview.chromium.org/11314008/diff/1/sync/sessions/status_controlle...
sync/sessions/status_controller.h:10: // state, and can be called only when the
SafetyRestriction is in effect.  Other
On 2012/10/26 21:41:46, rlarocque wrote:
> On 2012/10/26 21:14:30, Nicolas Zea wrote:
> > I think the more indepth description that was here previously is still
useful
> > right?
> 
> The only remaining use of the ModelSafeGroupRestriction is
> commit_id_projection().  The examples of conflict_progress() and commit_ids()
> are no longer valid.  The comments about session_state.h are no longer valid,
> since there is no longer any PerModelSafeGroupState.  Much of the "shared"
state
> are strictly increasing counters that are incremented on different threads, so
> the comment about requiring a lack of group restriction no longer applies.
> 
> I'd be open to re-wording this comment if you think it leaves out some
valuable
> information or is less clear than the original.  However, much of the original
> is out of date and had to be modified, if not deleted.  

"per-thread" is kind of implicit, the more explicit description for me is per
model safe group. I'd mainly just like to see the example provided previously
(i.e. using GROUP_UI) kept and just updated to use an appropriate field (e.g.
commit_id_projection). You're right, the rest of it isn't really applicable
anymore.

[tim (not reviewing), 2012-10-26 22:44:05]

I think the unittests are fine, have a comment below though.

Also, I'm hesitant to remove code (HasMoreToSync) that's only unused if we
ignore the fact that we have open bugs that discuss the need to rely on it (like
crbug.com/157955 and my comment in 111280), and I'm worried this will only
reduce the chances of us fixing that properly because it makes it that much more
work. 

For example, it seems like it'd be easier to rely on HasMoreToSync than do what
you proposed in the bug (jumping out of the commit loop, doing a GU, re-entering
the commit loop), now that commit is wholly enclosed in a single command. 
Removing this function means we'll inevitably drift further away from another
easy option.

http://codereview.chromium.org/11314008/diff/1/sync/protocol/get_updates_call...
File sync/protocol/get_updates_caller_info.proto (right):

http://codereview.chromium.org/11314008/diff/1/sync/protocol/get_updates_call...
sync/protocol/get_updates_caller_info.proto:22: // No longer sent as of M24.
Hmm.  I think we should still use this to denote cycles that occur due to
CONFLICT return codes.  We're using exponential backoff right now for those
cases, but that's out of convenience.  We should not backoff exponentially for
conflict errors, we should just retry.  And I could see it being useful to
distinguish those GetUpdates.

[rlarocque, 2012-10-31 23:48:20]

Patch rebased and updated.  PTAL.

http://codereview.chromium.org/11314008/diff/1/sync/protocol/get_updates_call...
File sync/protocol/get_updates_caller_info.proto (right):

http://codereview.chromium.org/11314008/diff/1/sync/protocol/get_updates_call...
sync/protocol/get_updates_caller_info.proto:22: // No longer sent as of M24.
On 2012/10/26 22:44:06, timsteele wrote:
> Hmm.  I think we should still use this to denote cycles that occur due to
> CONFLICT return codes.  We're using exponential backoff right now for those
> cases, but that's out of convenience.  We should not backoff exponentially for
> conflict errors, we should just retry.  And I could see it being useful to
> distinguish those GetUpdates.  

As discussed offline, I think it would be better to use a different value for
that purpose.  The newer clients would be scheduling those jobs based on
different reasoning.  They should use a new enum value to make it clear that's
what they are doing.

[tim (not reviewing), 2012-11-01 00:41:29]

LGTM with fixed tests

[commit-bot: I haz the power, 2012-11-01 18:39:22]

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

[commit-bot: I haz the power, 2012-11-01 19:38:55]

Change committed as 165474