sync: Process 'control' data separately

sync: Process 'control' data separately

The Nigori node's update application and conflict resolution code is
unique.  This change pulls the code for working with Nigori nodes out of
the regular ModelChangingSyncerCommands and into a separate function
named ApplyControlDataUpdates.

As part of this change, certain ApplyUpdatesCommandTest test cases have
been renamed and moved to ApplyControlDataUpdatesTest.

We will soon be introducing new data types that are special in their own
way.  This CL was written with this in mind.  It adds a new
ModelSafeGroup whose types are excluded from regular update application
and conflict resolution steps.  Membership in this group is controlled
through the 'IsControlType()' function in model_type.h.

BUG=122825, 111297

[rlarocque, 2012-06-21 02:06:59]

I'm still fighting with unit tests, and there are a few style violations and
FIXMEs outstanding.  However, this does represent the rough outline of how I
intend to implement this.

Please take a quick look, if you have time.

[Nicolas Zea, 2012-06-21 02:58:36]

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_metadata_...
File sync/engine/apply_metadata_updates.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_metadata_...
sync/engine/apply_metadata_updates.cc:53: if
(nigori_node.Get(IS_UNAPPLIED_UPDATE)) {
if (!nigori_node.Get(IS_UNAPPLIED_UPDATE))
  return;

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:172:
ApplyMetadataUpdates(session->context()->directory());
We can enforce the early application by just making sure that the the
passive/internal model worker is the first in the list. It might be cleaner to
have this method be part of ApplyUpdatesCommand's ModelChangeExecuteImpl, and
condition whether we should call this or the normal functionality by checking
which worker we're on?

http://codereview.chromium.org/10559104/diff/4002/sync/internal_api/public/en...
File sync/internal_api/public/engine/model_safe_worker.h (right):

http://codereview.chromium.org/10559104/diff/4002/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:38: GROUP_INTERNAL,      //
Models that require special handling on the sync
It's not clear to me why this is different (or needs a separate model worker)
than GROUP_PASSIVE? The whole point of passive is for those types that are
handled on the sync thread and don't have an associated native model.

Could we just get away with having the logic conditional on GROUP_INTERNAL be
conditional on GROUP_PASSIVE instead?

[rlarocque, 2012-06-21 18:23:27]

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:172:
ApplyMetadataUpdates(session->context()->directory());
On 2012/06/21 02:58:36, nzea wrote:
> We can enforce the early application by just making sure that the the
> passive/internal model worker is the first in the list. It might be cleaner to
> have this method be part of ApplyUpdatesCommand's ModelChangeExecuteImpl, and
> condition whether we should call this or the normal functionality by checking
> which worker we're on?

I'd rather not enforce an ordering constraint on the list.  I think it's
currently stored as a set, and many of the functions that work with sets of
model safe groups don't necessarily respect ordering.  

The ModelSafeWorker functions are supposed to be unaware of which group they're
on.  See crbug.com/121303.

http://codereview.chromium.org/10559104/diff/4002/sync/internal_api/public/en...
File sync/internal_api/public/engine/model_safe_worker.h (right):

http://codereview.chromium.org/10559104/diff/4002/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:38: GROUP_INTERNAL,      //
Models that require special handling on the sync
On 2012/06/21 02:58:36, nzea wrote:
> It's not clear to me why this is different (or needs a separate model worker)
> than GROUP_PASSIVE? The whole point of passive is for those types that are
> handled on the sync thread and don't have an associated native model.
> 
> Could we just get away with having the logic conditional on GROUP_INTERNAL be
> conditional on GROUP_PASSIVE instead?

I added this so I could exclude these types from regular processing in some
ModelChangingSyncerCommands.  I couldn't use GROUP_PASSIVE, since that is also
used for initializing other data types that eventually reside on other threads. 


Now that I see the effect this is having on tests, I'm not so sure it is the
right approach.

[rlarocque, 2012-06-21 20:15:27]

At lunch, Tim asked why I didn't simply remove the NIGORI type from the routing
info before/during ApplyUpdatesCommand and ResolveConflictsCommand.  That's a
good question.  It took me a while to remember the answer.

This won't work because the routing info doesn't have any direct effect.  The
session context shards model-specific information by *group*, not by type.  This
means that we will still attempt to apply any available nigori updates any time
the ApplyUpdatesCommand is run on the GROUP_PASSIVE ModelSafeGroup, regardless
of whether or not it is actually in routing info.

[tim (not reviewing), 2012-06-22 16:34:15]

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
sync/engine/apply_updates_command.cc:41:
groups_with_unapplied_updates.erase(GROUP_INTERNAL);
Hmm.  If we apply these types separately and before ApplyUpdates, what are the
cases where we would actually find unapplied updates for internal types here? 
Shouldn't they have already been processed?

Also, we should keep the naming consistent.  This patch uses 'INTERNAL' and
'Metadata' to refer to the same thing (e.g. to be consistent your step should be
ApplyInternalData, or you should make the group 'metadata'.)  Although we
already use 'internal' and 'metas' to mean other things :/ How about calling
these 'ControlData/GROUP_CONTROL', 'AccountSetup' or 'Base' or something.

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:172:
ApplyMetadataUpdates(session->context()->directory());
On 2012/06/21 18:23:27, rlarocque wrote:
> On 2012/06/21 02:58:36, nzea wrote:
> > We can enforce the early application by just making sure that the the
> > passive/internal model worker is the first in the list. It might be cleaner
to
> > have this method be part of ApplyUpdatesCommand's ModelChangeExecuteImpl,
and
> > condition whether we should call this or the normal functionality by
checking
> > which worker we're on?
> 
> I'd rather not enforce an ordering constraint on the list.  I think it's
> currently stored as a set, and many of the functions that work with sets of
> model safe groups don't necessarily respect ordering.  
> 
> The ModelSafeWorker functions are supposed to be unaware of which group
they're
> on.  See crbug.com/121303.  

Yeah, we shouldn't try to force ordering within the command.  If this needs to
happen before ApplyUpdatesCommand for all "real" types, then it should happen
before the apply updates step, in its own step.

[rlarocque, 2012-06-22 16:42:31]

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
sync/engine/apply_updates_command.cc:41:
groups_with_unapplied_updates.erase(GROUP_INTERNAL);
On 2012/06/22 16:34:15, timsteele wrote:
> Hmm.  If we apply these types separately and before ApplyUpdates, what are the
> cases where we would actually find unapplied updates for internal types here? 
> Shouldn't they have already been processed?

I think you're right.  Maybe we should assert that GROUP_INTERNAL is not in the
list?
 
> Also, we should keep the naming consistent.  This patch uses 'INTERNAL' and
> 'Metadata' to refer to the same thing (e.g. to be consistent your step should
be
> ApplyInternalData, or you should make the group 'metadata'.)  Although we
> already use 'internal' and 'metas' to mean other things :/ How about calling
> these 'ControlData/GROUP_CONTROL', 'AccountSetup' or 'Base' or something. 

I didn't like those names either.  I like GROUP_CONTROL, though.  I'll rename
accordingly.

[rlarocque, 2012-06-22 18:40:34]

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10559104/diff/4002/sync/engine/apply_updates_c...
sync/engine/apply_updates_command.cc:41:
groups_with_unapplied_updates.erase(GROUP_INTERNAL);
On 2012/06/22 16:42:31, rlarocque wrote:
> On 2012/06/22 16:34:15, timsteele wrote:
> > Hmm.  If we apply these types separately and before ApplyUpdates, what are
the
> > cases where we would actually find unapplied updates for internal types
here? 
> > Shouldn't they have already been processed?
> 
> I think you're right.  Maybe we should assert that GROUP_INTERNAL is not in
the
> list?

Actually, that won't work.  We release the transaction between
ApplyMetadataUpdates and ApplyUpdatesCommand.  A user with good timing could
update the nigori state between the two steps.

>  
> > Also, we should keep the naming consistent.  This patch uses 'INTERNAL' and
> > 'Metadata' to refer to the same thing (e.g. to be consistent your step
should
> be
> > ApplyInternalData, or you should make the group 'metadata'.)  Although we
> > already use 'internal' and 'metas' to mean other things :/ How about calling
> > these 'ControlData/GROUP_CONTROL', 'AccountSetup' or 'Base' or something. 
> 
> I didn't like those names either.  I like GROUP_CONTROL, though.  I'll rename
> accordingly.

[rlarocque, 2012-06-22 23:24:16]

Updated patch.  This one seems to actually work.

I've decided not to try to commit this week.  While it probably works just fine
as is, I think this patch should include some refactoring of the backend
initialization and migration logic to ensure that all GROUP_CONTROL types are
downloaded early.  That code will be rewritten when Nicolas re-lands his backend
intialization patches, so I'll wait until then before committing this.  

I also need to commit the patch that refactors ApplyUpdatesCommandUnittest
before this one.

[rlarocque, 2012-08-07 19:32:21]

This review has been obsoleted by the rebased and improved
http://codereview.chromium.org/10825137/.