Implement sync in the NonBlockingTypeProcessor

sync/engine/model_thread_sync_entity.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SYNC_ENGINE_MODEL_THREAD_SYNC_ENTITY_H_
+#define SYNC_ENGINE_MODEL_THREAD_SYNC_ENTITY_H_
+
+#include <string>
+
+#include "base/time/time.h"
+#include "sync/base/sync_export.h"
+#include "sync/engine/non_blocking_sync_common.h"
+#include "sync/protocol/sync.pb.h"
+
+namespace syncer {
+
+// This is the model thread's representation of a SyncEntity.
+//
+// The model thread sync entity receives updates from the model itself and
+// (asynchronously) from the sync server via the sync thread.  From the point
+// of view of this class, updates from the server take precedence over local
+// changes, though the model may be given an opportunity to overwrite this
+// decision later.
+//
+// Sync will try to commit this entity's data to the sync server and local
+// storage.
+//
+// Most of the logic related to those processes live outside this class.  This
+// class helps out a bit by offering some functions to serialize its data to
+// various formats and query the entity's status.
+class SYNC_EXPORT_PRIVATE ModelThreadSyncEntity {
+ public:
+  // Construct an instance representing a new locally-created item.
+  static ModelThreadSyncEntity* NewLocalItem(
+      const std::string& client_tag,
+      const sync_pb::EntitySpecifics& specifics,
+      base::Time now);
+
+  // Construct an instance representing an item newly received from the server.
+  static ModelThreadSyncEntity* FromServerUpdate(
+      const std::string& id,
+      const std::string& client_tag_hash,
+      const std::string& non_unique_name,
+      int64 version,
+      const sync_pb::EntitySpecifics& specifics,
+      bool deleted,
+      base::Time ctime,
+      base::Time mtime);
+
+  // TODO(rlarocque): Implement FromDisk constructor when we implement storage.
+
+  ~ModelThreadSyncEntity();
+
+  // Returns true if this data is out of sync with local storage.
+  bool IsWriteRequired() const;
+
+  // Returns true if this data is out of sync with the server.
+  // A commit may or may not be in progress at this time.
+  bool IsUnsynced() const;
+
+  // Returns true if this data is out of sync with the sync thread.
+  //
+  // There may or may not be a commit in progress for this item, but there's
+  // definitely no commit in progress for this (most up to date) version of
+  // this item.

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

mention that this is a subset of IsUnsynced?

[rlarocque, 2014/05/20 01:39:47]

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.

+  bool RequiresCommitRequest() const;
+
+  // Returns true if the specified update version does not contain new data.
+  bool UpdateIsReflection(int64 update_version) const;

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

Is update_version the same as server_version? I'd prefer to stick with
server_version for terminology in that case

[rlarocque, 2014/05/20 01:39:47]

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.

+
+  // Returns true if the specified update version conflicts with local changes.
+  bool UpdateIsInConflict(int64 update_version) const;
+
+  // Applies an update from the sync server.
+  //
+  // Overrides any local changes.  Check UpdateIsInConflict() before calling
+  // this function if you want to handle conflicts differently.
+  void ApplyUpdateFromServer(int64 update_version,
+                             bool deleted,
+                             const sync_pb::EntitySpecifics& specifics,
+                             base::Time mtime);
+
+  // Applies a local change to this item.
+  void MakeLocalChange(const std::string& client_tag,

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

why is tag necessary here and below? Aren't those immutable?

[rlarocque, 2014/05/20 01:39:47]

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.

+                       const sync_pb::EntitySpecifics& specifics);
+
+  // Applies a local deletion to this item.
+  void Delete(const std::string& client_tag);
+
+  // Initializes a message representing this item's uncommitted state
+  // to be forwarded to the sync server for committing.
+  void InitializeCommitRequestData(CommitRequestData* request) const;
+
+  // Notes that the current version of this item has been queued for commit.
+  void SetCommitRequestInProgress();
+
+  // Receives a successful commit response.
+  //
+  // Sucssful commit responses can overwrite an item's ID.
+  //
+  // Note that the receipt of a successful commit response does not necessarily
+  // unset IsUnsynced().  If many local changes occur in quick succession, it's
+  // possible that the committed item was already out of date by the time it
+  // reached the server.
+  void ReceiveCommitResponse(const std::string& id,
+                             int64 sequence_number,
+                             int64 response_version);
+
+ private:
+  ModelThreadSyncEntity(int64 sequence_number,
+                        int64 commit_requested_sequence_number,
+                        int64 acked_sequence_number,
+                        int64 base_version,
+                        bool is_dirty,
+                        const std::string& id,
+                        const std::string& client_tag,
+                        const std::string& client_tag_hash,
+                        const std::string& non_unique_name,
+                        const sync_pb::EntitySpecifics& specifics,
+                        bool deleted,
+                        base::Time ctime,
+                        base::Time mtime);
+
+  // A sequence number used to track in-progress commits.  Each local change
+  // increments this number.
+  int64 sequence_number_;
+
+  // The sequence number of the last item sent to the sync thread.
+  int64 commit_requested_sequence_number_;
+
+  // The sequence number of the last item known to be successfully committed.
+  int64 acked_sequence_number_;
+
+  // The server version on which this item is based.
+  //
+  // If there are no local changes, this is the version of the entity as we see
+  // it here.
+  //
+  // If there are local changes, this is the version of the entity on which
+  // those changes are based.
+  int64 base_version_;
+
+  // True if this entity is out of sync with local storage.
+  bool is_dirty_;
+
+  // The entity's ID.
+  //
+  // Most of the time, this is a server-assigned value.
+  //
+  // Prior to the item's first commit, we leave this value as an empty string.
+  // The initial ID for a newly created item has to meet certain uniqueness
+  // requirements, and we handle those on the sync thread.
+  std::string id_;
+
+  // The client tag of this entity.
+  //
+  // There are some cases where we don't know this value for a while.  It
+  // should be equal to the non_unique_name_, but I'd prefer to not rely on

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

tag and non_unique_name have no explicit relation

[rlarocque, 2014/05/20 01:39:47]

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

Also, this is now removed.  See below.

+  // that assumption.
+  //
+  // Any updates we receive from the server will contain a hash based on this
+  // value, but not hte client tag itself.  We will leave this field empty

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

nit: hte -> the

[rlarocque, 2014/05/20 01:39:47]

Done.

+  // until the client specifies our client_tag.
+  std::string client_tag_;

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

Is this field necessary at all? Why not just always use the hash?

[rlarocque, 2014/05/20 01:39:47]

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.

+
+  // A hash based on the client tag and model type.
+  // Used for various map lookups.  Should always be available.
+  std::string client_tag_hash_;
+
+  // A non-unique name associated with this entity.
+  //
+  // It is sometimes used for debugging.  Most of the time it's equal to the
+  // client ID.  We don't use it for much at the moment.  Required by the sync

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

Non unique name is entirely a UI/debug construct. That said, by client id I'm
assuming you mean client tag

[rlarocque, 2014/05/20 01:39:47]

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.

[Nicolas Zea, 2014/05/20 21:51:08]

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.

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

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.

+  // protocol, though as far as I know it doesn't do anything with the value.
+  std::string non_unique_name_;
+
+  // A protobuf filled with type-specific information.  Contains the most
+  // up-to-date specifics, whether it be from the server or a locally modified
+  // version.
+  sync_pb::EntitySpecifics specifics_;
+
+  // Whether or not the item is deleted.  The |specifics_| field may be empty
+  // if this flag is true.
+  bool deleted_;

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

Should this entity also have a ModelType field?

[rlarocque, 2014/05/20 01:39:47]

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

+
+  // Entity creation and modification timestamps.
+  // Assigned by the client and synced by the server, though the server usually
+  // doesn't bother to inspect their values.
+  base::Time ctime_;
+  base::Time mtime_;
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_ENGINE_MODEL_THREAD_SYNC_ENTITY_H_