Helium: Initial proto and Syncable interface definition

blimp/net/helium/syncable.h

+// Copyright 2016 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 BLIMP_NET_HELIUM_SYNCABLE_H_
+#define BLIMP_NET_HELIUM_SYNCABLE_H_
+
+#include <stdint.h>
+
+#include "base/callback.h"
+#include "blimp/common/proto/helium.pb.h"
+
+#include "vector_clock.h"

[CJ, 2016/09/29 20:36:55]

Nit: I think you want to give the full path to vector_clock relative to blimp.

[scf, 2016/09/29 23:13:39]

Done.

+
+namespace blimp {
+
+// SyncableObject and Syncable are conceptually very similar. Both are objects

[Sriram, 2016/09/29 18:52:21]

Not to bikeshed with names but I find SyncableObject and Syncable a bit
confusing. How about SyncableObject that contains SyncedMemeber?

[Brian Goldman, 2016/09/29 19:31:28]

Seconded that it's confusing. For me it's because "object" kind of means the
same thing as "instance." So a Syncable instance is a Syncable object but it's
not a SyncableObject o_O

[scf, 2016/09/29 23:13:38]

Not bikeshed at all. I was not super happy with the name. Agree. Will rename to
|SyncableObject| and |SyncableMember|

+// that will be synchronized between client server.

[Brian Goldman, 2016/09/29 19:31:28]

Maybe a little more detail here on what it means that objects are synchronized
between client and server. (Also "engine" is more precise than "server" in this
context, I think.) My understanding is:

1. There is a one-to-one relationship between instances on the client and
instances on the engine.
2. The values stored in client-engine pairs should eventually be equal.

If my understanding is wrong, all the more reason to document the correct
meaning :)

[scf, 2016/09/29 23:13:39]

Done.

+//
+// The main difference is that SyncableObject owns its lifetime, whereas

[Kevin M, 2016/09/29 20:01:16]

The lifetime is managed by an arbitrary entity which will vary depending on the
object impl.

[scf, 2016/09/29 23:13:39]

I find this a bit vague. Not entirely sure what you mean

+// Syncable depends on its SyncableObject parent.
+//
+// An example for GeoLocation would be:
+// GeoLocation : SyncableObject {
+//   * Frequency : Syncable
+//   * Position : Syncable
+// }
+//
+// VectorClocks from a Syncable can be compared with the vector clock from

[CJ, 2016/09/29 20:36:55]

This a bit confusing as well. Is the vector clock from the SyncableObject also 
a VectorClock? If this is the case, keep capitalization consistent. 

[scf, 2016/09/29 23:13:39]

Done.

+// the SyncableObject. This reduces the amount of state that can be kept.
+
+class SyncableObject {

[Sriram, 2016/09/29 18:52:21]

A SyncableObject does not control how often it is synced or is that something
that happens using some other mechanism? The interface currently works for pull
(i.e. the underlying sync layer pulls data from the objects) but does not when
object needs to push or notify the sync layer requesting something to be synced
(sync layer could still decide not to sync immediately).

For example - how will say GeoLocation convey that location has changed? 

[scf, 2016/09/29 23:13:39]

There will be a notification mechanism to tell the HeliumSyncManager about its
changes. Its not covered in this interface.

+ public:
+  using CreateChangesetCallback =
+      base::Callback<void(std::unique_ptr<helium::Changeset> changeset)>;

[Kevin M, 2016/09/29 20:01:16]

In general we don't specify parameter names in function signatures.

[scf, 2016/09/29 23:13:38]

Done.

+
+  explicit SyncableObject(const VectorClock& clock) : clock_(clock) {}
+
+  virtual ~SyncableObject(){}
+
+  // Constructs a changeset between the |from| revision and its current state.
+  // The Sync layer will encapsulate the changeset with details since |from|,
+  // but the Object is responsible for including any revision information
+  // additional to that expressed by the VectorClocks, that is necessary to
+  // detect and resolve conflicts.
+  // The computed changeset is returned asynchronously via |response_callback|.
+  virtual void CreateChangesetToCurrent(

[Kevin M, 2016/09/29 20:01:16]

Shouldn't these be overrides of Syncable instead of pure virtual methods?

[scf, 2016/09/29 23:13:38]

I thought about making the interfaces different as the SyncableObject would have
callbacks, whereas the |SyncableMember| wouldn't.

Since this is causing confusion. I will go ahead and merge them.

+      const VectorClock& from,
+      const CreateChangesetCallback& response_callback) = 0;
+
+  // Applies a |changeset| given as parameter to the contents of the
+  // Object. The VectorClocks |from| and |to| can be used to detect and resolve

[CJ, 2016/09/29 20:36:55]

Not following when u say "the Object". If you mean the object that derives from
Syncable, indicate Syncable object. 

[scf, 2016/09/29 23:13:38]

Done.

+  // concurrent change conflicts.
+  // The closure |done| its called when the state is applied.
+  virtual void ApplyChangeset(const VectorClock& from,
+                              const VectorClock& to,
+                              const helium::Changeset& changeset,
+                              const base::Closure& done) = 0;
+
+  // This call is an optimization that allows the source of the change

[Kevin M, 2016/09/29 20:01:16]

* This isn't a mere optimization; it's actually quite important!
* Use a factual explanation of what the method does at the top.
* Not sure if we need to point out that it's an optimization.
* "Let's say..." is too conversational, and the example seems out of place
compared to the comments for the other methods in this class.

[scf, 2016/09/29 23:13:39]

Done.

+  // delete anything before a given checkpoint. Lets say we know the receiver
+  // already received the change for VectorClock (2,3). The source now can
+  // delete any state that it needed to create a changeset from x to (2,3).
+  virtual void ReleaseCheckpointsBefore(const VectorClock& checkpoint) = 0;
+
+  VectorClock get_clock() { return clock_; }

[scf, 2016/09/29 18:26:53]

This is needed in order for the |Syncacle| objects whenever they modify their
state they update their |last_updated_| value. The rationale here is use a
"global" per SyncableObject clock which makes state management simpler. The
SyncManager just needs to keep track of 2 bits of information: The object id and
the |last_sync_time_|. Otherwise the sync manager will also need to remember the
|last_sync_time_| of a Syncable (which there will be many)

The other alternative that I also thought was have a local clock on the
|Syncable| and |SyncableObject| object. But this will complicate the design a
bit as for each change on a |Syncable| we would have to send the vector clock
info (As opposed of just sending per SyncableObject)

[Kevin M, 2016/09/29 20:01:16]

You are returning the clock by-value, which means that the Syncables are
operating on their own local VectorClocks?

This should probably be returning &clock_ instead.

[CJ, 2016/09/29 20:36:55]

Maybe put a little of this rationale in a comment, perhaps not here but rather
by the definition of |clock_|. 

[scf, 2016/09/29 23:13:39]

Done.

+
+ protected:
+  VectorClock clock_;
+};
+
+template <class ChangesetType>
+class Syncable {
+ public:
+  explicit Syncable(VectorClock* parent_clock) : parent_clock_(parent_clock) {}

[Kevin M, 2016/09/29 20:01:16]

There's a circular dependency in that we have a "parent clock" specified in a
base class constructor, but one of the subclasses is the provider of said parent
clock.

I would recommend structuring the hierarchy like this:

Syncable -- pure virtual interface, no fields, no constructor.
SyncableObject : public Syncable -- does not take a VectorClock.
SyncableFragment : public Syncable -- takes a SyncableObject* parent, which IMO
is more explicit about the notion of the Syncable hierarchy than sharing
VectorClocks.

[scf, 2016/09/29 23:13:39]

completely agree

+  virtual ~Syncable(){}
+
+  // Returns true if the object have been modified since |from|
+  virtual bool ModifiedSince(const VectorClock& from) = 0;
+
+  // See above definition in |SyncableObject|, the only difference is that
+  // this is a blocking call

[Kevin M, 2016/09/29 20:01:16]

Why are these all blocking?

[scf, 2016/09/29 23:13:39]

I should have checked with you offline before I wrote this. I thought I was
creating too much complexity when implementing a SyncableObject. but this is
causing a lot of confusion. I will go ahead and merge the interfaces

+  virtual void CreateChangesetToCurrent(const VectorClock& from,
+                                        ChangesetType* changeset) = 0;
+
+  // See above definition in |SyncableObject|, the only difference is that
+  // this is a blocking call
+  virtual void ApplyChangeset(const VectorClock& from,
+                              const VectorClock& to,
+                              const ChangesetType& changeset) = 0;
+
+  // See above definition in |SyncableObject|, the only difference is that
+  // this is a blocking call
+  virtual void ReleaseCheckpointsBefore(const VectorClock& checkpoint) = 0;
+
+ protected:
+  // Increments the parent clock and returns the new value. Should be used
+  // whenever a Syncable updates its state.
+  VectorClock IncrementParentClock() {
+    parent_clock_->IncrementLocal();
+    return *parent_clock_;
+  }
+
+  VectorClock parent_clock() { return *parent_clock_; }

[Kevin M, 2016/09/29 20:01:16]

Should return a pointer

[scf, 2016/09/29 23:13:39]

Done.

+
+ private:
+  VectorClock* parent_clock_;
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_HELIUM_SYNCABLE_H_