Implement sync in the NonBlockingTypeProcessor

Implement sync in the NonBlockingTypeProcessor

Introduces the NonBlockingTypeProcessor's sync logic.  When combined
with the NonBlockingTypeProcessorCore's sync logic (which will be
introduced in a follow-up commit), this will be an alternative to the
existing sync engine implemented with DirectoryUpdateHandler and
DirectoryCommitContributor.

Adds non_blocking_sync_common.h, which defines structs to be used to
pass messages between the processor and processor core.

Adds DataTypeState as a parameter to the processor to processor core
connection methods.  Eventually this will be used to initialize the
processor core with state that the processor loaded from disk.

Adds a lot of unit tests and unit test framework intrastructure.  The
NonBlockingTypeProcessor and NonBlockingTypeProcessorCore's
communications with each other will be very racy.  These tests are
intended to help manage the complexity this will cause by allowing us to
test all the possible race conditions individually.

BUG=351005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272329

[rlarocque, 2014-05-13 23:20:29]

Here's the first big chunk of the sync engine implementation.

Please review.

https://codereview.chromium.org/280983002/diff/1/sync/engine/model_thread_syn...
File sync/engine/model_thread_sync_entity_unittest.cc (right):

https://codereview.chromium.org/280983002/diff/1/sync/engine/model_thread_syn...
sync/engine/model_thread_sync_entity_unittest.cc:23: // Instead, we focus on
simple tests to make sure that variables are getting
It's debatable whether or not these tests should exist.

I was hoping they might shake out some bugs during development, but they all
passed on the first try.  That may indicate that these tests are too trivial to
be worth the maintenance effort.

https://codereview.chromium.org/280983002/diff/1/sync/engine/non_blocking_syn...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/1/sync/engine/non_blocking_syn...
sync/engine/non_blocking_sync_common.h:6: #define
SYNC_ENGINE_NON_BLOCKING_SYNC_COMMON_H_
These definitions are fairly trivial and only used in the Processor and
ProcessorCore, so I figured there was no harm in grouping them together.  Let me
know if you'd prefer to split them out one per file.

I'm open to suggestions for a better name for this file.

https://codereview.chromium.org/280983002/diff/1/sync/engine/non_blocking_syn...
sync/engine/non_blocking_sync_common.h:17: struct SYNC_EXPORT_PRIVATE
DataTypeState {
This struct might deserve its own file more than the others do, since a lot of
the classes on the processor -> processor core "Connect" path end up needing to
include it.

https://codereview.chromium.org/280983002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor.h (right):

https://codereview.chromium.org/280983002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor.h:63: void Put(const std::string&
client_tag,
Yes, we can split this up into Add() and Update() cases.  I'll add that
functionality when we wrap this class in a SyncableService (or whatever
SyncableService-like interface we end up using).

https://codereview.chromium.org/280983002/diff/1/sync/sessions/model_type_reg...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/1/sync/sessions/model_type_reg...
sync/sessions/model_type_registry.cc:22: class
NonBlockingTypeProcessorCoreWrapper
I'm on the fence about whether or not this should be in a separate file.  Since
no one else references it, and it's simple enough that it doesn't really need
thorough testing, I figured it would make more sense to keep it here.

[Nicolas Zea, 2014-05-15 21:58:54]

Does this patch account for any of the concerns/discussions we had the other
day, particularly w.r.t Attachments and using something along the lines of a
SyncData/SyncEntity object?

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:22: class
NonBlockingTypeProcessorCoreWrapper
Is this that much better than having NonBlockingTypeCore implement the interface
directly? I find this quite confusing, and it took me a while to figure out how
the core interface actually related to the core.

Why not use the normal Impl approach over this?

[Nicolas Zea, 2014-05-15 21:59:28]

+CC Nick/Pavel

[rlarocque, 2014-05-15 22:28:45]

On 2014/05/15 21:58:54, Nicolas Zea wrote:
> Does this patch account for any of the concerns/discussions we had the other
> day, particularly w.r.t Attachments and using something along the lines of a
> SyncData/SyncEntity object?
> 

No, net yet.  That was intentional.

For SyncData and SyncEntity, I was hoping that you might be convinced of the
utility of having a variety of representations for the sync node when you see
how the ModelThreadSyncEntity and non_blocking_sync_common.h types are used in
practice.

If you're still not convinced, then it won't be hard to replace those structs'
SyncEntity, is_del, etc. members with a struct that contains SyncEntity, is_del,
etc.  Most of the code will look exactly the same.

For attachments, I think we agreed that it won't be hard to add that feature
later.  It's just an extra field to pass around, and (presumably) a bunch more
tests.  I think it's safe to defer that for a future CL, since it is possible to
build a working sync engine without that feature.

[rlarocque, 2014-05-15 22:28:54]

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:22: class
NonBlockingTypeProcessorCoreWrapper
On 2014/05/15 21:58:55, Nicolas Zea wrote:
> Is this that much better than having NonBlockingTypeCore implement the
interface
> directly? I find this quite confusing, and it took me a while to figure out
how
> the core interface actually related to the core.
> 
> Why not use the normal Impl approach over this?

The most practical reason is that it allows me to hide both the object and the
thread that it lives on.  The NonBlockingTypeProcessor doesn't need to use
PostTask() when it sends messages to the NonBlockingTypeProcessorCore because
the wrapper hides that detail.

This makes unit tests a lot simpler.  It's possible to remove all task posting
from the interaction with the MockNonBlockingTypeProcessorCore.

[maniscalco, 2014-05-16 20:37:19]

On 2014/05/15 22:28:45, rlarocque wrote:
> On 2014/05/15 21:58:54, Nicolas Zea wrote:
> > Does this patch account for any of the concerns/discussions we had the other
> > day, particularly w.r.t Attachments and using something along the lines of a
> > SyncData/SyncEntity object?
> > 
> 
> No, net yet.  That was intentional.
> 
> For SyncData and SyncEntity, I was hoping that you might be convinced of the
> utility of having a variety of representations for the sync node when you see
> how the ModelThreadSyncEntity and non_blocking_sync_common.h types are used in
> practice.
> 
> If you're still not convinced, then it won't be hard to replace those structs'
> SyncEntity, is_del, etc. members with a struct that contains SyncEntity,
is_del,
> etc.  Most of the code will look exactly the same.
> 
> For attachments, I think we agreed that it won't be hard to add that feature
> later.  It's just an extra field to pass around, and (presumably) a bunch more
> tests.  I think it's safe to defer that for a future CL, since it is possible
to
> build a working sync engine without that feature.

I'm ambivalent.  It does seem a little risky to build the new sync engine
without incorporating attachment support into the API.  One the one hand, I'm
concerned that we could end up with a finished sync engine that lacks attachment
support and therefore can't be used to replace the old sync engine.  On the
other hand, I don't want to slow down the progress of this work (or the
attachment work) by coupling them too early.

Richard, at some point in the near future (Q2/Q3?), will you go back and add
attachment support to this new sync engine?

[rlarocque, 2014-05-19 20:32:56]

On 2014/05/16 20:37:19, maniscalco wrote:
> On 2014/05/15 22:28:45, rlarocque wrote:
> > On 2014/05/15 21:58:54, Nicolas Zea wrote:
> > > Does this patch account for any of the concerns/discussions we had the
other
> > > day, particularly w.r.t Attachments and using something along the lines of
a
> > > SyncData/SyncEntity object?
> > > 
> > 
> > No, net yet.  That was intentional.
> > 
> > For SyncData and SyncEntity, I was hoping that you might be convinced of the
> > utility of having a variety of representations for the sync node when you
see
> > how the ModelThreadSyncEntity and non_blocking_sync_common.h types are used
in
> > practice.
> > 
> > If you're still not convinced, then it won't be hard to replace those
structs'
> > SyncEntity, is_del, etc. members with a struct that contains SyncEntity,
> is_del,
> > etc.  Most of the code will look exactly the same.
> > 
> > For attachments, I think we agreed that it won't be hard to add that feature
> > later.  It's just an extra field to pass around, and (presumably) a bunch
more
> > tests.  I think it's safe to defer that for a future CL, since it is
possible
> to
> > build a working sync engine without that feature.
> 
> I'm ambivalent.  It does seem a little risky to build the new sync engine
> without incorporating attachment support into the API.  One the one hand, I'm
> concerned that we could end up with a finished sync engine that lacks
attachment
> support and therefore can't be used to replace the old sync engine.  On the
> other hand, I don't want to slow down the progress of this work (or the
> attachment work) by coupling them too early.
> 
> Richard, at some point in the near future (Q2/Q3?), will you go back and add
> attachment support to this new sync engine?

We've discussed this offline.  The plan is to revisit this after this CL lands. 
In particular, we can take another look at it when we start working on the
non-blocking sync's interface with the model.

[Nicolas Zea, 2014-05-19 23:54:15]

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
File sync/engine/model_thread_sync_entity.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:65: // this item.
mention that this is a subset of IsUnsynced?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:69: bool UpdateIsReflection(int64
update_version) const;
Is update_version the same as server_version? I'd prefer to stick with
server_version for terminology in that case

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:84: void MakeLocalChange(const
std::string& client_tag,
why is tag necessary here and below? Aren't those immutable?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:158: // should be equal to the
non_unique_name_, but I'd prefer to not rely on
tag and non_unique_name have no explicit relation

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:162: // value, but not hte client tag
itself.  We will leave this field empty
nit: hte -> the

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:164: std::string client_tag_;
Is this field necessary at all? Why not just always use the hash?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:173: // client ID.  We don't use it for
much at the moment.  Required by the sync
Non unique name is entirely a UI/debug construct. That said, by client id I'm
assuming you mean client tag

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:184: bool deleted_;
Should this entity also have a ModelType field?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:23: std::string parent_id;
why do we have parent_id and next_id here? Aren't those per item, rather than
per type?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:33: int64 sequence_number;
what is sequence number?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:38: sync_pb::EntitySpecifics specifics;
what about non unique name?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:57: int64 response_version;
server_version?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:108: ModelThreadSyncEntity* entity =
ModelThreadSyncEntity::NewLocalItem(
this should probably be using scoped_ptr

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:127: DLOG(WARNING) << "Attempted to
delete missing item."
Given our discussion about these constraints being useful for identifying
datatype errors, I think it might be good for this to have some sort of
consequence (possibly later on, once error handling is added).

Similarly, that might be an argument for separating put into add/update

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:62: // Requests that an item be stored
in sync.
Will this also update?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:69: // Sends all commit requests that
are due to be sent to the sync thread.
Does this imply that Put/Delete don't trigger talking to the sync thread? Or
should this be a private method?

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:74: void ReceiveCommitResponse(const
DataTypeState& type_state,
nit: something like OnCommitCompletion seems like a better name

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:79: void ReceiveUpdateResponse(const
DataTypeState& type_state,
nit: OnUpdateReceived

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:104: // conntected to sync.
nit: connected

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_unittest.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_unittest.cc:474:
TEST_F(NonBlockingTypeProcessorTest, DeleteServerUnknown_RacyCommitResponse) {
Is this actually testing anything in its current state?

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:22: class
NonBlockingTypeProcessorCoreWrapper
On 2014/05/15 22:28:54, rlarocque wrote:
> On 2014/05/15 21:58:55, Nicolas Zea wrote:
> > Is this that much better than having NonBlockingTypeCore implement the
> interface
> > directly? I find this quite confusing, and it took me a while to figure out
> how
> > the core interface actually related to the core.
> > 
> > Why not use the normal Impl approach over this?
> 
> The most practical reason is that it allows me to hide both the object and the
> thread that it lives on.  The NonBlockingTypeProcessor doesn't need to use
> PostTask() when it sends messages to the NonBlockingTypeProcessorCore because
> the wrapper hides that detail.
> 
> This makes unit tests a lot simpler.  It's possible to remove all task posting
> from the interaction with the MockNonBlockingTypeProcessorCore.

How about changing the naming then. It seems that the Interface is really a sync
engine construct. Will it eventually have more than just RequestCommits? If not,
naming it something based on the fact that it's a commit delegator of some sort
makes more sense.

Part of my confusion is that typically calling something XInterface means that
the X is actually implementing the interface, which isn't really true. This is
just a delegation class.

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:146: (void)data_type_state;
what is this doing?

[rlarocque, 2014-05-20 01:39:46]

Here's a bunch of responses and an updated patch.

Your comment about non_unique_name made me realize that I'm not doing a very
good job of testing it at the moment.  I'll try to fix that in a future patch.

The rest of the code should be ready for another review pass.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
File sync/engine/model_thread_sync_entity.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:65: // this item.
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> mention that this is a subset of IsUnsynced?

It isn't.  RequiresCommitRequest() is true when the sync thread doesn't yet know
about changes to this item.  IsUnsynced() is set when we haven't received a
commit response acknowledging that this item has been committed to the server.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:69: bool UpdateIsReflection(int64
update_version) const;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Is update_version the same as server_version? I'd prefer to stick with
> server_version for terminology in that case

Sort of.  The new sync engine splits the definition of that term in half.

Previously, server_version represented the most recently received version from
the server, as known to both the sync thread and model thread.  In the new
system, the version known here may be behind the version known to the sync
thread.

I figured it was more accurate to call this update version, since it is the
version delivered with the item we're currently working with.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:84: void MakeLocalChange(const
std::string& client_tag,
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> why is tag necessary here and below? Aren't those immutable?

Sort of.

Consider the case where we receive an update from the server.  At that point, we
don't know the client tag.  We only learn about the tag when the model makes a
change to this item for the first time.

I ended up removing this.  See comments further down.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:158: // should be equal to the
non_unique_name_, but I'd prefer to not rely on
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> tag and non_unique_name have no explicit relation

Yep.  That's why I'm not relying on that assumption here.

Also, this is now removed.  See below.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:162: // value, but not hte client tag
itself.  We will leave this field empty
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> nit: hte -> the

Done.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:164: std::string client_tag_;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Is this field necessary at all? Why not just always use the hash?

That is a valid alternative.  

I was hoping to present an interface where the tag is less opaque, but the
update from server case has mostly put an end to that.  Previous iterations of
this code assumed some that some model-specific knowledge could be injected into
this layer, but that's not going to work if our interface to the model is based
on Syncable API.

I guess I'll just delete it, and all the code associated with it.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:173: // client ID.  We don't use it for
much at the moment.  Required by the sync
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Non unique name is entirely a UI/debug construct. That said, by client id I'm
> assuming you mean client tag

Correct.  s/ID/tag/.

These values do get committed to and returned by the server, so I think it's a
little more than a UI and debug construct.  The sync protocol has explicit
support for it.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:184: bool deleted_;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Should this entity also have a ModelType field?

It's not necessary because the Processor and ProcessorCore know their own types.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:23: std::string parent_id;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> why do we have parent_id and next_id here? Aren't those per item, rather than
> per type?

parent_id: Other than bookmarks, sync doesn't really support hierarchy.  I have
no intention of attempting to support hierarchy for non-blocking sync types.

If you look at the structure of the Directory today, you'll see that all items
of the same model type (except bookmarks) have the same parent.  That's the
behavior I'm looking to mimic with this code.

According to the server devs, I could leave the parent_id unset for most
"unique_client_tag" types with no ill effects.  I'm not quite sure I believe
them yet.  I intend to start by mimicing the old behavior; we can experiment
with empty parent IDs later.

next_id: This is a completely different thing.  The name is misleading, though
it does match the name it had in directory sync.  This is the counter used as a
nonce to ensure that no two items have the same client-assigned ID.

According to the server devs, this is also useless.  Once again, I'm including
it just so I can mimic the old behavior.

See
https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/dire...

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:33: int64 sequence_number;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> what is sequence number?

A new concept for non-blocking sync.  Feel free to suggest a better name.

With non-blocking sync, it's possible that there are many variations of a
particular item with outstanding commit requests at any given time.  These can
not be identified by the base version or the server version alone.

For example, consider an item that is modified 10 times while we're offline. 
The model thread will send 10 commit requests to the sync thread.  When it
receives a commit response, how does it know if that response refers to the 1st
or 10th request?

My solution is to use a sequence number.  Think of it as a different
implementation of the "is_unsynced" and "syncing" bits.

The sequence number is never forwarded to the sync server (yet).  It's up to the
sync thread to handle some of the mapping.  There may be some benefits to
sending it all the way to the server, particularly if crashes are frequent, but
it's probably not worth exploring those possibilities just yet.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:38: sync_pb::EntitySpecifics specifics;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> what about non unique name?

That's a bug.

I must have had the ProcessorCore extract it from the client_tag.  Now that the
client_tag is no longer available, I've added an explicit non_unique_name field.
 Which is the way it should have been implemented in the first place.

Fixed.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:57: int64 response_version;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> server_version?

I'm a bit reluctant to use the term 'server_version', since it could be out of
date by the time this field is read.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:108: ModelThreadSyncEntity* entity =
ModelThreadSyncEntity::NewLocalItem(
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> this should probably be using scoped_ptr

Done.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:127: DLOG(WARNING) << "Attempted to
delete missing item."
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Given our discussion about these constraints being useful for identifying
> datatype errors, I think it might be good for this to have some sort of
> consequence (possibly later on, once error handling is added).
> 
> Similarly, that might be an argument for separating put into add/update

Agreed.  I consider the Put/Delete interface to be a placeholder for now.  I'll
fix it when we start to work on the model-facing interface.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:62: // Requests that an item be stored
in sync.
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Will this also update?

Update as in fetch updates?  No, there's no guarantee of that.  It's certainly
not going to happen synchronously, that's for sure.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:69: // Sends all commit requests that
are due to be sent to the sync thread.
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Does this imply that Put/Delete don't trigger talking to the sync thread? Or
> should this be a private method?

Made private.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:74: void ReceiveCommitResponse(const
DataTypeState& type_state,
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> nit: something like OnCommitCompletion seems like a better name

Done.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:79: void ReceiveUpdateResponse(const
DataTypeState& type_state,
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> nit: OnUpdateReceived

Done.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.h:104: // conntected to sync.
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> nit: connected

Done.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_unittest.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_unittest.cc:474:
TEST_F(NonBlockingTypeProcessorTest, DeleteServerUnknown_RacyCommitResponse) {
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> Is this actually testing anything in its current state?

It's exercising the internals of this code in an unusual way, though you'd need
a white-box perspective to know that.  It proves the syncer won't crash or leak
memory when this happens.

I'd prefer to test externally observable side effects, like whether or not this
scenario generates a spurious delete notification.  That testing will have to
wait until we have an interface that emits notifications when deletions are
received.

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:22: class
NonBlockingTypeProcessorCoreWrapper
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> On 2014/05/15 22:28:54, rlarocque wrote:
> > On 2014/05/15 21:58:55, Nicolas Zea wrote:
> > > Is this that much better than having NonBlockingTypeCore implement the
> > interface
> > > directly? I find this quite confusing, and it took me a while to figure
out
> > how
> > > the core interface actually related to the core.
> > > 
> > > Why not use the normal Impl approach over this?
> > 
> > The most practical reason is that it allows me to hide both the object and
the
> > thread that it lives on.  The NonBlockingTypeProcessor doesn't need to use
> > PostTask() when it sends messages to the NonBlockingTypeProcessorCore
because
> > the wrapper hides that detail.
> > 
> > This makes unit tests a lot simpler.  It's possible to remove all task
posting
> > from the interaction with the MockNonBlockingTypeProcessorCore.
> 
> How about changing the naming then. It seems that the Interface is really a
sync
> engine construct. Will it eventually have more than just RequestCommits? If
not,
> naming it something based on the fact that it's a commit delegator of some
sort
> makes more sense.
> 
> Part of my confusion is that typically calling something XInterface means that
> the X is actually implementing the interface, which isn't really true. This is
> just a delegation class.

Right.  Let's discuss naming tomorrow.  There will be a few of these, so we
should figure out some standard way to name the pattern.

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:146: (void)data_type_state;
On 2014/05/19 23:54:15, Nicolas Zea wrote:
> what is this doing?

Silences an unused variable warning.  I seem to remember that certain platforms
would choke on that warning in certain contexts.

This is my way of saying: "Yes, I know this variable isn't used anywhere, and
that's OK".  Hopefully it's informative to both readers and the compiler.

[rlarocque, 2014-05-20 17:29:33]

Added some expectations for non_unique_names.

I decided against plumbing adding more support for non_unique_names in the test
framework at the moment.  They're only observable from debugging interfaces,
which haven't been built yet.  We can add support for entries where
non_unique_name != client_tag and some additional tests when those interfaces
become available.

[Nicolas Zea, 2014-05-20 21:51:07]

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
File sync/engine/model_thread_sync_entity.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:173: // client ID.  We don't use it for
much at the moment.  Required by the sync
On 2014/05/20 01:39:47, rlarocque wrote:
> On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > Non unique name is entirely a UI/debug construct. That said, by client id
I'm
> > assuming you mean client tag
> 
> Correct.  s/ID/tag/.
> 
> These values do get committed to and returned by the server, so I think it's a
> little more than a UI and debug construct.  The sync protocol has explicit
> support for it.

Right, it has support, but just for ui/debug usage. I don't think it's worth
mentioning any relationship to the client tag though, just mention that it's for
UI/debug usage, and possibly that it gets overwritten when the entity is
encrypted.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:23: std::string parent_id;
On 2014/05/20 01:39:47, rlarocque wrote:
> On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > why do we have parent_id and next_id here? Aren't those per item, rather
than
> > per type?
> 
> parent_id: Other than bookmarks, sync doesn't really support hierarchy.  I
have
> no intention of attempting to support hierarchy for non-blocking sync types.
> 
> If you look at the structure of the Directory today, you'll see that all items
> of the same model type (except bookmarks) have the same parent.  That's the
> behavior I'm looking to mimic with this code.
> 
> According to the server devs, I could leave the parent_id unset for most
> "unique_client_tag" types with no ill effects.  I'm not quite sure I believe
> them yet.  I intend to start by mimicing the old behavior; we can experiment
> with empty parent IDs later.
> 
> next_id: This is a completely different thing.  The name is misleading, though
> it does match the name it had in directory sync.  This is the counter used as
a
> nonce to ensure that no two items have the same client-assigned ID.
> 
> According to the server devs, this is also useless.  Once again, I'm including
> it just so I can mimic the old behavior.
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/dire...

I think it would be clearer to rename these two fields then (in addition to
adding comments). parent_id could be type_root_id, and next_id could be
next_client_id

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:33: int64 sequence_number;
On 2014/05/20 01:39:47, rlarocque wrote:
> On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > what is sequence number?
> 
> A new concept for non-blocking sync.  Feel free to suggest a better name.
> 
> With non-blocking sync, it's possible that there are many variations of a
> particular item with outstanding commit requests at any given time.  These can
> not be identified by the base version or the server version alone.
> 
> For example, consider an item that is modified 10 times while we're offline. 
> The model thread will send 10 commit requests to the sync thread.  When it
> receives a commit response, how does it know if that response refers to the
1st
> or 10th request?
> 
> My solution is to use a sequence number.  Think of it as a different
> implementation of the "is_unsynced" and "syncing" bits.
> 
> The sequence number is never forwarded to the sync server (yet).  It's up to
the
> sync thread to handle some of the mapping.  There may be some benefits to
> sending it all the way to the server, particularly if crashes are frequent,
but
> it's probably not worth exploring those possibilities just yet.

The name is fine, I mainly want comments

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:146: (void)data_type_state;
On 2014/05/20 01:39:47, rlarocque wrote:
> On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > what is this doing?
> 
> Silences an unused variable warning.  I seem to remember that certain
platforms
> would choke on that warning in certain contexts.
> 
> This is my way of saying: "Yes, I know this variable isn't used anywhere, and
> that's OK".  Hopefully it's informative to both readers and the compiler.

I don't think the warning happens for parameters, just locally declared
variables. Can you double check that this is necessary?

https://codereview.chromium.org/280983002/diff/60001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/60001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:21: sync_pb::DataTypeProgressMarker
progress_marker;
I'd prefer to have comments for all these fields.

https://codereview.chromium.org/280983002/diff/80001/sync/engine/model_thread...
File sync/engine/model_thread_sync_entity.h (right):

https://codereview.chromium.org/280983002/diff/80001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:34: static ModelThreadSyncEntity*
NewLocalItem(
these should both return scoped_ptr's

[rlarocque, 2014-05-20 22:29:21]

Patch updated.  PTAL.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
File sync/engine/model_thread_sync_entity.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/model_thread...
sync/engine/model_thread_sync_entity.h:173: // client ID.  We don't use it for
much at the moment.  Required by the sync
On 2014/05/20 21:51:08, Nicolas Zea wrote:
> On 2014/05/20 01:39:47, rlarocque wrote:
> > On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > > Non unique name is entirely a UI/debug construct. That said, by client id
> I'm
> > > assuming you mean client tag
> > 
> > Correct.  s/ID/tag/.
> > 
> > These values do get committed to and returned by the server, so I think it's
a
> > little more than a UI and debug construct.  The sync protocol has explicit
> > support for it.
> 
> Right, it has support, but just for ui/debug usage. I don't think it's worth
> mentioning any relationship to the client tag though, just mention that it's
for
> UI/debug usage, and possibly that it gets overwritten when the entity is
> encrypted.

I've updated the comment.

I don't see any harm in mentioning the relation with the client tag, as long as
we're careful to mention this is not guaranteed and may change in the future. 
Also added some information about encryption.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:23: std::string parent_id;
On 2014/05/20 21:51:08, Nicolas Zea wrote:
> On 2014/05/20 01:39:47, rlarocque wrote:
> > On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > > why do we have parent_id and next_id here? Aren't those per item, rather
> than
> > > per type?
> > 
> > parent_id: Other than bookmarks, sync doesn't really support hierarchy.  I
> have
> > no intention of attempting to support hierarchy for non-blocking sync types.
> > 
> > If you look at the structure of the Directory today, you'll see that all
items
> > of the same model type (except bookmarks) have the same parent.  That's the
> > behavior I'm looking to mimic with this code.
> > 
> > According to the server devs, I could leave the parent_id unset for most
> > "unique_client_tag" types with no ill effects.  I'm not quite sure I believe
> > them yet.  I intend to start by mimicing the old behavior; we can experiment
> > with empty parent IDs later.
> > 
> > next_id: This is a completely different thing.  The name is misleading,
though
> > it does match the name it had in directory sync.  This is the counter used
as
> a
> > nonce to ensure that no two items have the same client-assigned ID.
> > 
> > According to the server devs, this is also useless.  Once again, I'm
including
> > it just so I can mimic the old behavior.
> > 
> > See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/dire...
> 
> I think it would be clearer to rename these two fields then (in addition to
> adding comments). parent_id could be type_root_id, and next_id could be
> next_client_id

Renamed and added lots of comments for this struct.

https://codereview.chromium.org/280983002/diff/20001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:33: int64 sequence_number;
On 2014/05/20 21:51:08, Nicolas Zea wrote:
> On 2014/05/20 01:39:47, rlarocque wrote:
> > On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > > what is sequence number?
> > 
> > A new concept for non-blocking sync.  Feel free to suggest a better name.
> > 
> > With non-blocking sync, it's possible that there are many variations of a
> > particular item with outstanding commit requests at any given time.  These
can
> > not be identified by the base version or the server version alone.
> > 
> > For example, consider an item that is modified 10 times while we're offline.

> > The model thread will send 10 commit requests to the sync thread.  When it
> > receives a commit response, how does it know if that response refers to the
> 1st
> > or 10th request?
> > 
> > My solution is to use a sequence number.  Think of it as a different
> > implementation of the "is_unsynced" and "syncing" bits.
> > 
> > The sequence number is never forwarded to the sync server (yet).  It's up to
> the
> > sync thread to handle some of the mapping.  There may be some benefits to
> > sending it all the way to the server, particularly if crashes are frequent,
> but
> > it's probably not worth exploring those possibilities just yet.
> 
> The name is fine, I mainly want comments

Added a comment.  It points readers to look for the places where the value is
used, since that's going to be the most up-to-date source of information.  I
don't trust maintainers (including myself) to update the comments here if the
behavior in model_thread_sync_entity.cc changes.

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/280983002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:146: (void)data_type_state;
On 2014/05/20 21:51:08, Nicolas Zea wrote:
> On 2014/05/20 01:39:47, rlarocque wrote:
> > On 2014/05/19 23:54:15, Nicolas Zea wrote:
> > > what is this doing?
> > 
> > Silences an unused variable warning.  I seem to remember that certain
> platforms
> > would choke on that warning in certain contexts.
> > 
> > This is my way of saying: "Yes, I know this variable isn't used anywhere,
and
> > that's OK".  Hopefully it's informative to both readers and the compiler.
> 
> I don't think the warning happens for parameters, just locally declared
> variables. Can you double check that this is necessary?

Updated in the latest patch.  Let's see if the trybots accept it.

https://codereview.chromium.org/280983002/diff/60001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/60001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:21: sync_pb::DataTypeProgressMarker
progress_marker;
On 2014/05/20 21:51:08, Nicolas Zea wrote:
> I'd prefer to have comments for all these fields.

I've added a bunch around DataTypeState.  I think those definitions are likely
to be stable.

I'm a bit more reluctant to add lots of comments around the others.  I see them
as being like SyncBackendHost's DoInitializeOptions: function parameters that
have grown so numerous that they had to be refactored into a struct.

They should be well documented in the places where they're used.  It's hard to
document them here, since they don't actually do anything here.  Also, the
documentation near where they're used will be more likely to be updated and
maintained if the meaning of these fields changes.

[Nicolas Zea, 2014-05-21 22:20:07]

LGTM!

https://codereview.chromium.org/280983002/diff/100001/sync/engine/non_blockin...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/100001/sync/engine/non_blockin...
sync/engine/non_blocking_sync_common.h:26: // A data type context.  Sent to the
server in every commit or updat request.
nit: update

[rlarocque, 2014-05-21 22:39:33]

https://codereview.chromium.org/280983002/diff/100001/sync/engine/non_blockin...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/280983002/diff/100001/sync/engine/non_blockin...
sync/engine/non_blocking_sync_common.h:26: // A data type context.  Sent to the
server in every commit or updat request.
On 2014/05/21 22:20:08, Nicolas Zea wrote:
> nit: update

Done.

[rlarocque, 2014-05-21 22:39:41]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-21 22:43:50]

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

[commit-bot: I haz the power, 2014-05-22 09:04:16]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-22 09:30:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 09:30:12]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[rlarocque, 2014-05-22 17:18:19]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:21:06]

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

[commit-bot: I haz the power, 2014-05-22 21:40:13]

Change committed as 272329