Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2382533002:
Helium: Initial proto and Syncable interface definition (Closed)
|
Created:
4 years, 2 months ago by scf Modified:
4 years ago CC:
anandc+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, cbentzel+watch_chromium.org, dtrainor+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial definition of proto messages and Syncable interface definition.
BUG=650461
Committed: https://crrev.com/d10e6413f024668200243747059fcbb9ef0e00aa
Cr-Commit-Position: refs/heads/master@{#423919}
Patch Set 1 #Patch Set 2 : remainder stuff #Patch Set 3 : remainder stuff #Patch Set 4 : remainder stuff #Patch Set 5 : remainder stuff #Patch Set 6 : Initial proto and Syncable interface definition #
Total comments: 54
Patch Set 7 : Addressing initial feedback #
Total comments: 8
Patch Set 8 : Couple minor tweaks #
Total comments: 57
Patch Set 9 : allthefeedback #Patch Set 10 : git cl upload #Patch Set 11 : https://memegen.googleplex.com/4141705 #
Total comments: 26
Patch Set 12 : addressing feedback #
Total comments: 16
Patch Set 13 : missed one comma #Patch Set 14 : moar feedback #
Total comments: 5
Patch Set 15 : updating with feedback #
Total comments: 6
Patch Set 16 : making Apply async based on last discussions, and some minor comment updates #Patch Set 17 : changes as discussed offline with @wez and @kevin #
Total comments: 5
Patch Set 18 : temp before sending out #Patch Set 19 : internal before sharing #Patch Set 20 : remainder stufft #
Total comments: 28
Patch Set 21 : addressing latest round of feedback #
Total comments: 3
Patch Set 22 : merging with master #
Total comments: 1
Patch Set 23 : test changes #Messages
Total messages: 70 (23 generated)
Description was changed from ========== remainder stuff Merge branch 'master' into syncable_api_new syncable api BUG= ========== to ========== Initial definition of proto messages and Syncable interface definition. BUG=650461 ==========
scf@chromium.org changed reviewers: + kmarshall@chromium.org, lethalantidote@chromium.org, steimel@chromium.org
some 2 questions/thoughts https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: VectorClock get_clock() { return clock_; } 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) https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:161: VectorClock last_sync_remote_; One thought also is have this bit of information in the SyncableObject itself. We might as well add the object id in there for making state management simpler. @kmarshall: thoughts?
sriramsr@chromium.org changed reviewers: + sriramsr@chromium.org
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:17: // SyncableObject and Syncable are conceptually very similar. Both are objects Not to bikeshed with names but I find SyncableObject and Syncable a bit confusing. How about SyncableObject that contains SyncedMemeber? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:32: class SyncableObject { 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? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:18: VectorClock::Comparison VectorClock::CompareTo(const VectorClock& other) const { Nit: Might be worth adding Unit tests to for the methods. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:19: DCHECK(local_revision_ >= other.local_revision()); Can you add comment as to why Comparison() expects the other.local_revision() to be greater than or equal to this objects? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:25: class VectorClock { Add comment on threading expectation of the object. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:65: VectorClock Invert(); Can you update the comment to tell when it would be used?
bgoldman@chromium.org changed reviewers: + bgoldman@chromium.org
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:17: // SyncableObject and Syncable are conceptually very similar. Both are objects On 2016/09/29 18:52:21, Sriram wrote: > Not to bikeshed with names but I find SyncableObject and Syncable a bit > confusing. How about SyncableObject that contains SyncedMemeber? > 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 https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:18: // that will be synchronized between client server. 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 :)
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:17: // ilustrate a sample implementation. Will replace with mocks. Actually I favor keeping IntValueSyncable around for documentation purposes.
Haven't reviewed the unit tests yet, will look at them once the syncable.h feedback is addressed. https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:27: message LegacyChangeset { I think we should cross this particular bridge when we get to it - we don't know what the exact shape and requirements of this object will be. What about a TestChangeset? https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:32: // be one for each feature that requires serialization. feature => Helium Object? https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:43: message Message { "Message" is too overloaded - HeliumMessage? https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:47: VectorClock from = 1; Use blank lines to separate commented field blocks. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:20: // The main difference is that SyncableObject owns its lifetime, whereas The lifetime is managed by an arbitrary entity which will vary depending on the object impl. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:35: base::Callback<void(std::unique_ptr<helium::Changeset> changeset)>; In general we don't specify parameter names in function signatures. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:47: virtual void CreateChangesetToCurrent( Shouldn't these be overrides of Syncable instead of pure virtual methods? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:60: // This call is an optimization that allows the source of the change * 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. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: VectorClock get_clock() { return clock_; } On 2016/09/29 18:26:53, scf wrote: > 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) 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. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: explicit Syncable(VectorClock* parent_clock) : parent_clock_(parent_clock) {} 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. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:82: // this is a blocking call Why are these all blocking? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:104: VectorClock parent_clock() { return *parent_clock_; } Should return a pointer
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:13: #include "vector_clock.h" Nit: I think you want to give the full path to vector_clock relative to blimp. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:29: // VectorClocks from a Syncable can be compared with the vector clock from This a bit confusing as well. Is the vector clock from the SyncableObject also a VectorClock? If this is the case, keep capitalization consistent. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:52: // Object. The VectorClocks |from| and |to| can be used to detect and resolve Not following when u say "the Object". If you mean the object that derives from Syncable, indicate Syncable object. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: VectorClock get_clock() { return clock_; } On 2016/09/29 18:26:53, scf wrote: > 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) Maybe put a little of this rationale in a comment, perhaps not here but rather by the definition of |clock_|. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:169: TEST_F(SyncableTest, Test1) { Usually we want to give our Test names a little more description. What are you testing here? Also, we seem to only be testing one aspect of your class. Perhaps more tests? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add "Message" at the end of the VectorClock proto name to make this explicit.
thanks for all the feedback. keep'em coming https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:27: message LegacyChangeset { On 2016/09/29 20:01:16, Kevin M wrote: > I think we should cross this particular bridge when we get to it - we don't know > what the exact shape and requirements of this object will be. > > What about a TestChangeset? Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:32: // be one for each feature that requires serialization. On 2016/09/29 20:01:16, Kevin M wrote: > feature => Helium Object? Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:43: message Message { On 2016/09/29 20:01:16, Kevin M wrote: > "Message" is too overloaded - HeliumMessage? Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/common/proto/hel... blimp/common/proto/helium.proto:47: VectorClock from = 1; On 2016/09/29 20:01:16, Kevin M wrote: > Use blank lines to separate commented field blocks. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:13: #include "vector_clock.h" On 2016/09/29 20:36:55, CJ wrote: > Nit: I think you want to give the full path to vector_clock relative to blimp. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:17: // SyncableObject and Syncable are conceptually very similar. Both are objects On 2016/09/29 18:52:21, Sriram wrote: > Not to bikeshed with names but I find SyncableObject and Syncable a bit > confusing. How about SyncableObject that contains SyncedMemeber? > Not bikeshed at all. I was not super happy with the name. Agree. Will rename to |SyncableObject| and |SyncableMember| https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:18: // that will be synchronized between client server. On 2016/09/29 19:31:28, Brian Goldman wrote: > 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 :) Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:20: // The main difference is that SyncableObject owns its lifetime, whereas On 2016/09/29 20:01:16, Kevin M wrote: > The lifetime is managed by an arbitrary entity which will vary depending on the > object impl. I find this a bit vague. Not entirely sure what you mean https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:29: // VectorClocks from a Syncable can be compared with the vector clock from On 2016/09/29 20:36:55, CJ wrote: > This a bit confusing as well. Is the vector clock from the SyncableObject also > a VectorClock? If this is the case, keep capitalization consistent. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:32: class SyncableObject { On 2016/09/29 18:52:21, Sriram wrote: > 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? There will be a notification mechanism to tell the HeliumSyncManager about its changes. Its not covered in this interface. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:35: base::Callback<void(std::unique_ptr<helium::Changeset> changeset)>; On 2016/09/29 20:01:16, Kevin M wrote: > In general we don't specify parameter names in function signatures. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:47: virtual void CreateChangesetToCurrent( On 2016/09/29 20:01:16, Kevin M wrote: > Shouldn't these be overrides of Syncable instead of pure virtual methods? 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. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:52: // Object. The VectorClocks |from| and |to| can be used to detect and resolve On 2016/09/29 20:36:55, CJ wrote: > Not following when u say "the Object". If you mean the object that derives from > Syncable, indicate Syncable object. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:60: // This call is an optimization that allows the source of the change On 2016/09/29 20:01:16, Kevin M wrote: > * 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. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: VectorClock get_clock() { return clock_; } On 2016/09/29 20:36:55, CJ wrote: > On 2016/09/29 18:26:53, scf wrote: > > 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) > > Maybe put a little of this rationale in a comment, perhaps not here but rather > by the definition of |clock_|. Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: explicit Syncable(VectorClock* parent_clock) : parent_clock_(parent_clock) {} On 2016/09/29 20:01:16, Kevin M wrote: > 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. completely agree https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:82: // this is a blocking call On 2016/09/29 20:01:16, Kevin M wrote: > Why are these all blocking? 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 https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable.h:104: VectorClock parent_clock() { return *parent_clock_; } On 2016/09/29 20:01:16, Kevin M wrote: > Should return a pointer Done. https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:169: TEST_F(SyncableTest, Test1) { On 2016/09/29 20:36:55, CJ wrote: > Usually we want to give our Test names a little more description. What are you > testing here? > > Also, we seem to only be testing one aspect of your class. Perhaps more tests? Good catch. I'm not really testing much here other than the test code and my assumptions. So I will rename this to DemoAPITest https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; On 2016/09/29 20:36:55, CJ wrote: > Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add > "Message" at the end of the VectorClock proto name to make this explicit. I was following the convention of other proto files. They neither specify a namespace of a suffix in the class name. Thoughts? https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:65: VectorClock Invert(); On 2016/09/29 18:52:21, Sriram wrote: > Can you update the comment to tell when it would be used? Done.
https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // Syncable depends on its SyncableObject parent. Replace "Syncable" with "SyncableMember" here.
https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable.cc:25: template class SyncableMember<int>; is there a way to write this template instantiation in a cleaner way? https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:88: base::Unretained(this), value1.get()); I tried passing the std::unique_ptr<int> here without success, any pointers are appreciated.
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; On 2016/09/29 23:13:39, scf wrote: > On 2016/09/29 20:36:55, CJ wrote: > > Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add > > "Message" at the end of the VectorClock proto name to make this explicit. > > I was following the convention of other proto files. They neither specify a > namespace of a suffix in the class name. Thoughts? The file names leave off the "message" part, but the actual messages defined in the proto file have "Message". Example: https://cs.chromium.org/chromium/src/blimp/common/proto/ime.proto?rcl=1475162...
https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable.cc:25: template class SyncableMember<int>; On 2016/09/29 23:20:08, scf wrote: > is there a way to write this template instantiation in a cleaner way? AFAIK, you either have to do this, define all the methods in your .h file, or include this file at the end of your .h file https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:88: base::Unretained(this), value1.get()); On 2016/09/29 23:20:08, scf wrote: > I tried passing the std::unique_ptr<int> here without success, any pointers are > appreciated. If you're going to pass a unique_ptr, you'll need to use std::move(), though afaik you won't be able to use value1.get() or *value1 after using std::move. If you're not giving up ownership of the object, you'll want to pass a pointer (i.e. value1.get())
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; On 2016/09/29 23:44:00, CJ wrote: > On 2016/09/29 23:13:39, scf wrote: > > On 2016/09/29 20:36:55, CJ wrote: > > > Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add > > > "Message" at the end of the VectorClock proto name to make this explicit. > > > > I was following the convention of other proto files. They neither specify a > > namespace of a suffix in the class name. Thoughts? > > The file names leave off the "message" part, but the actual messages defined in > the proto file have "Message". > Example: > https://cs.chromium.org/chromium/src/blimp/common/proto/ime.proto?rcl=1475162... HeliumMessage sounds good? https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable.cc:25: template class SyncableMember<int>; On 2016/09/30 16:43:58, steimel wrote: > On 2016/09/29 23:20:08, scf wrote: > > is there a way to write this template instantiation in a cleaner way? > > AFAIK, you either have to do this, define all the methods in your .h file, or > include this file at the end of your .h file The problem is that chromium style complained about having this constructor in the .h file as it became "Complex" https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // Syncable depends on its SyncableObject parent. On 2016/09/29 23:18:03, Brian Goldman wrote: > Replace "Syncable" with "SyncableMember" here. #goodcatch https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/120001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:88: base::Unretained(this), value1.get()); On 2016/09/30 16:43:58, steimel wrote: > On 2016/09/29 23:20:08, scf wrote: > > I tried passing the std::unique_ptr<int> here without success, any pointers > are > > appreciated. > > If you're going to pass a unique_ptr, you'll need to use std::move(), though > afaik you won't be able to use value1.get() or *value1 after using std::move. If > you're not giving up ownership of the object, you'll want to pass a pointer > (i.e. value1.get()) Weird. I saw some code passing std::unique_ptr<Foo> around on callbacks without using |get()|
https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:7: syntax = "proto3"; add blank line above this. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:15: // Vector clock that is used to get the partial order of changes add trailing period. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:17: // blimp/net/helium/vector_clock.h add double slash before blimp https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:29: // A "union" of the allowed types that are going to be serialized. There will Suggestion: "A union of serializable Changeset types." https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:31: // For example: GeoLocation, EngineSettings, etc. No need to provide an example; the oneof will be self explanatory once it gets filled in. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:33: oneof data { Choose a more specific name than "data"? https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:46: // Provides the local view of the vector-clock following application of add blank line before this https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.cc:21: // Instantiate the template for int (Unit test uses it) This approach doesn't scale - unit tests need it for now, but in the future, we'll need to update this list for all product callers as well. Put the templated code inline in the .h instead. If there is substantial common code that is template type agnostic, we can use helper functions, or a non-templated base class. For small method bodies, don't bother. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:41: Syncable() {} Don't need to add ctor - class doesn't have any complex fields. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:52: const CreateChangesetCallback& response_callback) = 0; Sorry - it now occurs to me that this should probably be synchronous. The sync manager will only be calling CreateChangeset* in response to State object modifications, which means that the State has all the information it needs to generate a changeset w/o threadhopping. So, remove the callback parameter and associated type alias? :\ https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:61: const ChangesetType& changeset, We should take this as a unique_ptr so that we can shuffle it around across threads safely and w/o copying. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: explicit SyncableObject(const VectorClock& clock); Maybe add a no-arg constructor that just uses a zeroed clock. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:78: VectorClock& get_clock() { return clock_; } * Inline getters don't need a get_ prefix. * References are only used for immutable values. If the caller is going to modify the value, then we need to return it by bare pointer. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:81: // The clock is needed in order for the |SyncableMember| objects whenever Remove space before The https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:85: VectorClock clock_; Add DISALLOW_COPY_AND_ASSIGN(SyncableObject); https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:102: SyncableObject* parent_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:25: // We initialize the last time we modified with the parent clock This is already made pretty clear by the code - recommend removing the comment. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:31: // my current clock. "my"? Also, the sentence doesn't parse. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:49: // Update our clock to the latest clock add newline above https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:61: // Increment the parent clock and update our last_modified_ value add newline above https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:65: int get_value() { return value_; } value() https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:84: int value1 = 0; In practice, we will only be setting values in the proto if those values are worth sending. I recommend moving the set_value1, set_value2 calls into the conditional blocks and removing these value1, value2 locals. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:120: base::Bind(&ParentObjectSyncable::OnDummy, base::Unretained(this)); base::DoNothing https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:147: IntValueSyncable child2_; DISALLOW yada yada https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:156: parent_remote_(last_sync_remote_) {} add newline below this https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:62: helium::VectorClock CreateProto(); Recommend "ToProto()" ?
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; On 2016/09/30 17:26:23, scf wrote: > On 2016/09/29 23:44:00, CJ wrote: > > On 2016/09/29 23:13:39, scf wrote: > > > On 2016/09/29 20:36:55, CJ wrote: > > > > Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add > > > > "Message" at the end of the VectorClock proto name to make this explicit. > > > > > > I was following the convention of other proto files. They neither specify a > > > namespace of a suffix in the class name. Thoughts? > > > > The file names leave off the "message" part, but the actual messages defined > in > > the proto file have "Message". > > Example: > > > https://cs.chromium.org/chromium/src/blimp/common/proto/ime.proto?rcl=1475162... > > HeliumMessage sounds good? Well, this is for a VectorClock message right? So it should be VectorClockMessage.
https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:58: helium::VectorClock result; On 2016/09/30 19:47:12, CJ wrote: > On 2016/09/30 17:26:23, scf wrote: > > On 2016/09/29 23:44:00, CJ wrote: > > > On 2016/09/29 23:13:39, scf wrote: > > > > On 2016/09/29 20:36:55, CJ wrote: > > > > > Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add > > > > > "Message" at the end of the VectorClock proto name to make this > explicit. > > > > > > > > I was following the convention of other proto files. They neither specify > a > > > > namespace of a suffix in the class name. Thoughts? > > > > > > The file names leave off the "message" part, but the actual messages defined > > in > > > the proto file have "Message". > > > Example: > > > > > > https://cs.chromium.org/chromium/src/blimp/common/proto/ime.proto?rcl=1475162... > > > > HeliumMessage sounds good? > > Well, this is for a VectorClock message right? So it should be > VectorClockMessage. talked offline. Suffixing everything to contain Message. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:7: syntax = "proto3"; On 2016/09/30 18:34:36, Kevin M wrote: > add blank line above this. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:15: // Vector clock that is used to get the partial order of changes On 2016/09/30 18:34:36, Kevin M wrote: > add trailing period. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:17: // blimp/net/helium/vector_clock.h On 2016/09/30 18:34:36, Kevin M wrote: > add double slash before blimp Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:29: // A "union" of the allowed types that are going to be serialized. There will On 2016/09/30 18:34:36, Kevin M wrote: > Suggestion: "A union of serializable Changeset types." Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:31: // For example: GeoLocation, EngineSettings, etc. On 2016/09/30 18:34:36, Kevin M wrote: > No need to provide an example; the oneof will be self explanatory once it gets > filled in. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:33: oneof data { On 2016/09/30 18:34:36, Kevin M wrote: > Choose a more specific name than "data"? couldn't really think of anything really other than |data| or |value|. Since the caller will be doing something like changeset()->data()->legacy() https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:46: // Provides the local view of the vector-clock following application of On 2016/09/30 18:34:36, Kevin M wrote: > add blank line before this Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.cc:21: // Instantiate the template for int (Unit test uses it) On 2016/09/30 18:34:36, Kevin M wrote: > This approach doesn't scale - unit tests need it for now, but in the future, > we'll need to update this list for all product callers as well. > > Put the templated code inline in the .h instead. If there is substantial common > code that is template type agnostic, we can use helper functions, or a > non-templated base class. For small method bodies, don't bother. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:41: Syncable() {} On 2016/09/30 18:34:36, Kevin M wrote: > Don't need to add ctor - class doesn't have any complex fields. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:52: const CreateChangesetCallback& response_callback) = 0; On 2016/09/30 18:34:37, Kevin M wrote: > Sorry - it now occurs to me that this should probably be synchronous. The sync > manager will only be calling CreateChangeset* in response to State object > modifications, which means that the State has all the information it needs to > generate a changeset w/o threadhopping. So, remove the callback parameter and > associated type alias? :\ Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:61: const ChangesetType& changeset, On 2016/09/30 18:34:36, Kevin M wrote: > We should take this as a unique_ptr so that we can shuffle it around across > threads safely and w/o copying. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: explicit SyncableObject(const VectorClock& clock); On 2016/09/30 18:34:36, Kevin M wrote: > Maybe add a no-arg constructor that just uses a zeroed clock. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:78: VectorClock& get_clock() { return clock_; } On 2016/09/30 18:34:36, Kevin M wrote: > * Inline getters don't need a get_ prefix. > * References are only used for immutable values. If the caller is going to > modify the value, then we need to return it by bare pointer. done. using the proto convention (mutable_name) https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:81: // The clock is needed in order for the |SyncableMember| objects whenever On 2016/09/30 18:34:36, Kevin M wrote: > Remove space before The Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:85: VectorClock clock_; On 2016/09/30 18:34:36, Kevin M wrote: > Add DISALLOW_COPY_AND_ASSIGN(SyncableObject); Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable.h:102: SyncableObject* parent_; On 2016/09/30 18:34:36, Kevin M wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:25: // We initialize the last time we modified with the parent clock On 2016/09/30 18:34:37, Kevin M wrote: > This is already made pretty clear by the code - recommend removing the comment. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:31: // my current clock. On 2016/09/30 18:34:37, Kevin M wrote: > "my"? > > Also, the sentence doesn't parse. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:49: // Update our clock to the latest clock On 2016/09/30 18:34:37, Kevin M wrote: > add newline above Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:61: // Increment the parent clock and update our last_modified_ value On 2016/09/30 18:34:37, Kevin M wrote: > add newline above Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:65: int get_value() { return value_; } On 2016/09/30 18:34:37, Kevin M wrote: > value() Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:84: int value1 = 0; On 2016/09/30 18:34:37, Kevin M wrote: > In practice, we will only be setting values in the proto if those values are > worth sending. I recommend moving the set_value1, set_value2 calls into the > conditional blocks and removing these value1, value2 locals. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:120: base::Bind(&ParentObjectSyncable::OnDummy, base::Unretained(this)); On 2016/09/30 18:34:37, Kevin M wrote: > base::DoNothing Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:147: IntValueSyncable child2_; On 2016/09/30 18:34:37, Kevin M wrote: > DISALLOW yada yada Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:156: parent_remote_(last_sync_remote_) {} On 2016/09/30 18:34:37, Kevin M wrote: > add newline below this Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:62: helium::VectorClock CreateProto(); On 2016/09/30 18:34:37, Kevin M wrote: > Recommend "ToProto()" ? Done.
lgtm
https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... blimp/common/proto/helium.proto:39: // A message encapsulates the actual Changeset with the identifier of the This is already self-evident by the contents of the struct. Maybe go with a higher level description of what this message is used for. https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... blimp/common/proto/helium.proto:56: // The changeset that contain the actual changes missing a trailing period https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:17: // SyncableObject and SyncableMember are conceptually very similar. Include a description about what a Syncable<> is here. Then drill down into SyncableObject/Member before those are declared. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:18: // Both are objects that will be synchronized between client and engine. This seems to pigeonhole in the differences and similarities between the two too quickly. I think it would be useful to describe the subclasses in standalone terms instead of relative term, because we are making relative comparisons between two concepts that are new in this file. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:69: SyncableObject(); What will this be used for? https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:73: const VectorClock& clock() { return clock_; } Method should be const. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: VectorClock* mutable_clock() { return &clock_; } Method should be const. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:82: VectorClock clock_; If we are using a global clock, then we should take this as a bare pointer from the creator and store the pointer as a field. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:94: // Returns true if the object have been modified since |from| trailing period. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:100: VectorClock IncrementParentClock(); Should we also have a method for getting the parent's current VectorClock? Then we can make parent_ into a private field. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:61: // Create the ProtoBuf object corresponding to this object "proto message"; trailing period; const https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:64: // Inverts the local and remote components respectively Trailing period. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:67: VectorClock Invert(); const
https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... blimp/common/proto/helium.proto:39: // A message encapsulates the actual Changeset with the identifier of the On 2016/10/03 21:44:26, Kevin M wrote: > This is already self-evident by the contents of the struct. Maybe go with a > higher level description of what this message is used for. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/common/proto/hel... blimp/common/proto/helium.proto:56: // The changeset that contain the actual changes On 2016/10/03 21:44:26, Kevin M wrote: > missing a trailing period Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:17: // SyncableObject and SyncableMember are conceptually very similar. On 2016/10/03 21:44:26, Kevin M wrote: > Include a description about what a Syncable<> is here. Then drill down into > SyncableObject/Member before those are declared. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:18: // Both are objects that will be synchronized between client and engine. On 2016/10/03 21:44:26, Kevin M wrote: > This seems to pigeonhole in the differences and similarities between the two too > quickly. I think it would be useful to describe the subclasses in standalone > terms instead of relative term, because we are making relative comparisons > between two concepts that are new in this file. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:69: SyncableObject(); On 2016/10/03 21:44:26, Kevin M wrote: > What will this be used for? Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:73: const VectorClock& clock() { return clock_; } On 2016/10/03 21:44:26, Kevin M wrote: > Method should be const. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:75: VectorClock* mutable_clock() { return &clock_; } On 2016/10/03 21:44:26, Kevin M wrote: > Method should be const. cannot return pointer of member in a const method https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:82: VectorClock clock_; On 2016/10/03 21:44:26, Kevin M wrote: > If we are using a global clock, then we should take this as a bare pointer from > the creator and store the pointer as a field. yes it will be done in a separate cl as discussed offline https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:94: // Returns true if the object have been modified since |from| On 2016/10/03 21:44:26, Kevin M wrote: > trailing period. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/synca... blimp/net/helium/syncable.h:100: VectorClock IncrementParentClock(); On 2016/10/03 21:44:26, Kevin M wrote: > Should we also have a method for getting the parent's current VectorClock? > > Then we can make parent_ into a private field. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:61: // Create the ProtoBuf object corresponding to this object On 2016/10/03 21:44:26, Kevin M wrote: > "proto message"; trailing period; const Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:64: // Inverts the local and remote components respectively On 2016/10/03 21:44:26, Kevin M wrote: > Trailing period. Done. https://codereview.chromium.org/2382533002/diff/200001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:67: VectorClock Invert(); On 2016/10/03 21:44:26, Kevin M wrote: > const Done.
https://codereview.chromium.org/2382533002/diff/220001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/common/proto/hel... blimp/common/proto/helium.proto:14: package blimp.helium; Recommend using the package "blimp.proto", so that in C++, we refer to these types as "proto::TestChangesetMessage". helium::TestChangesetMessage is ambiguous b/c a lot of Helium implementation is written in C++, too. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:12: #include "blimp/common/proto/helium.pb.h" Use forward declaration for ChangesetMessage? https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // 1. There is a one-to-one relationship between instances on the client and Give this ordered list a quick sentence for context. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:114: // Actual implementations are inline because of template instantiation. No need to write this https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:19: class IntValueSyncable : public SyncableMember<int> { FakeSyncable<int> ? https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:98: if (child1_value != 0) { Shouldn't we be calling ModifiedSince() instead of checking against magic numbers? https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:128: parent_local_(last_sync_local_), How does this work? last_sync_X_ is passed by-value and therefore will not be updated as parent_X_ changes. Please beef up the test case so that this omission gets caught. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:143: TEST_F(SyncableTest, DemoAPITest) { More descriptive test name?
https://codereview.chromium.org/2382533002/diff/220001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/common/proto/hel... blimp/common/proto/helium.proto:14: package blimp.helium; On 2016/10/03 22:30:35, Kevin M wrote: > Recommend using the package "blimp.proto", so that in C++, we refer to these > types as "proto::TestChangesetMessage". helium::TestChangesetMessage is > ambiguous b/c a lot of Helium implementation is written in C++, too. Done. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:12: #include "blimp/common/proto/helium.pb.h" On 2016/10/03 22:30:35, Kevin M wrote: > Use forward declaration for ChangesetMessage? Done. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // 1. There is a one-to-one relationship between instances on the client and On 2016/10/03 22:30:35, Kevin M wrote: > Give this ordered list a quick sentence for context. Done. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable.h:114: // Actual implementations are inline because of template instantiation. On 2016/10/03 22:30:35, Kevin M wrote: > No need to write this Done. https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:19: class IntValueSyncable : public SyncableMember<int> { On 2016/10/03 22:30:35, Kevin M wrote: > FakeSyncable<int> ? renamed to |FakeIntSyncable|. Making a template doesn't seem very useful if its been used for a single type parameter https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:98: if (child1_value != 0) { On 2016/10/03 22:30:35, Kevin M wrote: > Shouldn't we be calling ModifiedSince() instead of checking against magic > numbers? No. We cannot call ModifiedSince(). The other peer might not have been modified. The idea I had was using |has_fieldname| from protobuf. But since its a scalar value its not generated (And i didn't want to inflate the proto definition too much) https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:128: parent_local_(last_sync_local_), On 2016/10/03 22:30:35, Kevin M wrote: > How does this work? last_sync_X_ is passed by-value and therefore will not be > updated as parent_X_ changes. Please beef up the test case so that this omission > gets caught. Agree its not needed. The idea was to pass the initial time https://codereview.chromium.org/2382533002/diff/220001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:143: TEST_F(SyncableTest, DemoAPITest) { On 2016/10/03 22:30:35, Kevin M wrote: > More descriptive test name? Done.
perumaal@chromium.org changed reviewers: + perumaal@chromium.org
https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:19: uint32 local_revision = 1; Following the semantics for timestamps, would it better to make this uint64? (i.e. epoch timestamp in ms). https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... blimp/net/helium/syncable.h:54: // The computed changeset is returned asynchronously as a return parameter. Synchronously right? Unless I missed a base::Callback somewhere... https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... blimp/net/helium/syncable.h:107: VectorClock IncrementParentClock(); Please see my comments below. Seems like this separation of concerns is not clear or incorrect to me. The child should ideally not have any back-links to the parent. Typically the parent node has all the information to 'push' information to the children (and has the decision-making based on its one clock ideally): Otherwise, you would have this 'chatty' API between the parent and the child; while also having an extremely complex interaction as soon as you add more than 5 members to the parent. (Probably ok to discuss/fix in a future CL but wanted to call this out early on) https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:26: bool ModifiedSince(const VectorClock& from) override { This seems backward. The parent seems to know how to manage its child member Syncables. If so, the delegation from the parent to child back to parent seems confusing/backwards. parent::ApplyChangeset calls child::ModifiedSince which calls parent::parent_clock()::local_revision(). All of this information is available from the parent itself (as in, it maintains the master vector clock). In a way, I am wondering if we can remove all the 'child members have a direct pointer to their parent' which seems dangerous in general and looks like an inversion in separation of concerns. From the parent's perspective, one could presumably write code that had all the vector clock logic isolated within the parent object itself instead of having to seep it through the children - where the logic becomes distributed between the parent and each of the child members.
https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:19: uint32 local_revision = 1; On 2016/10/04 00:07:36, perumaal wrote: > Following the semantics for timestamps, would it better to make this uint64? > (i.e. epoch timestamp in ms). +1 int32s and int64s are encoded on the wire as varints, so this is giving us a measure of safety at no network cost. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:33: oneof data { On 2016/09/30 22:13:23, scf wrote: > On 2016/09/30 18:34:36, Kevin M wrote: > > Choose a more specific name than "data"? > > couldn't really think of anything really other than |data| or |value|. Since the > caller will be doing something like > > changeset()->data()->legacy() > "payload" ?
https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:19: uint32 local_revision = 1; On 2016/10/04 00:49:37, Kevin M wrote: > On 2016/10/04 00:07:36, perumaal wrote: > > Following the semantics for timestamps, would it better to make this uint64? > > (i.e. epoch timestamp in ms). > > +1 > > int32s and int64s are encoded on the wire as varints, so this is giving us a > measure of safety at no network cost. Done. https://codereview.chromium.org/2382533002/diff/140001/blimp/common/proto/hel... blimp/common/proto/helium.proto:33: oneof data { On 2016/10/04 00:49:37, Kevin M wrote: > On 2016/09/30 22:13:23, scf wrote: > > On 2016/09/30 18:34:36, Kevin M wrote: > > > Choose a more specific name than "data"? > > > > couldn't really think of anything really other than |data| or |value|. Since > the > > caller will be doing something like > > > > changeset()->data()->legacy() > > > > "payload" ? Done. https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... blimp/net/helium/syncable.h:107: VectorClock IncrementParentClock(); On 2016/10/04 00:07:36, perumaal wrote: > Please see my comments below. Seems like this separation of concerns is not > clear or incorrect to me. > The child should ideally not have any back-links to the parent. Typically the > parent node has all the information to 'push' information to the children (and > has the decision-making based on its one clock ideally): Otherwise, you would > have this 'chatty' API between the parent and the child; while also having an > extremely complex interaction as soon as you add more than 5 members to the > parent. > > (Probably ok to discuss/fix in a future CL but wanted to call this out early on) Yep. I agree. Thats what we discussed in the meeting Monday. We are going to have a single clock and a "clock generator". I asked Kevin to do in a separate diff which I started working. https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/260001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:26: bool ModifiedSince(const VectorClock& from) override { On 2016/10/04 00:07:37, perumaal wrote: > This seems backward. > The parent seems to know how to manage its child member Syncables. If so, the > delegation from the parent to child back to parent seems confusing/backwards. > > parent::ApplyChangeset calls child::ModifiedSince which calls > parent::parent_clock()::local_revision(). > > All of this information is available from the parent itself (as in, it maintains > the master vector clock). > > In a way, I am wondering if we can remove all the 'child members have a direct > pointer to their parent' which seems dangerous in general and looks like an > inversion in separation of concerns. > > From the parent's perspective, one could presumably write code that had all the > vector clock logic isolated within the parent object itself instead of having to > seep it through the children - where the logic becomes distributed between the > parent and each of the child members. > `parent::ApplyChangeset calls child::ModifiedSince which calls parent::parent_clock()::local_revision(). All of this information is available from the parent itself (as in, it maintains the master vector clock). ` false. ModifiedSince uses it own clock to check if it was modified since the last sync. Look at line 53. It gets modified every time we set the value. Thus it belongs here imo. Again, related to the clock comment, we will use a clock generator and remove the notion of parent here. hang tight for the next diff :)
https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // Changesets is a state that is propagated between client and server to Suggestion: Objects exchange Changesets between the client and server to keep their peer counterparts eventually synchronized. https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... blimp/net/helium/syncable.h:64: std::unique_ptr<ChangesetType> changeset) = 0; This one's going to have to be async. We need to give the policy layer a chance to apply the state mutation before we move on to the next changeset. https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:61: // Create the proto message object corresponding to this object. Message, not message object?
https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... blimp/net/helium/syncable.h:24: // Changesets is a state that is propagated between client and server to On 2016/10/04 17:28:12, Kevin M wrote: > Suggestion: Objects exchange Changesets between the client and server to keep > their peer counterparts eventually synchronized. Done. https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/synca... blimp/net/helium/syncable.h:64: std::unique_ptr<ChangesetType> changeset) = 0; On 2016/10/04 17:28:11, Kevin M wrote: > This one's going to have to be async. We need to give the policy layer a chance > to apply the state mutation before we move on to the next changeset. After discussing offline with @kevin and @wez we decided having 2 apis, sync and async. And the Async having a strawman implementation. https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2382533002/diff/280001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:61: // Create the proto message object corresponding to this object. On 2016/10/04 17:28:12, Kevin M wrote: > Message, not message object? Done.
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:61: VectorClock last_modified_; Should we make this a protected member at the SyncableMember level and then have a default implementation of ModifiedSince? That way we don't have to implement/test the same ModifiedSince logic in each subclass
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:61: VectorClock last_modified_; On 2016/10/05 00:57:38, steimel wrote: > Should we make this a protected member at the SyncableMember level and then have > a default implementation of ModifiedSince? That way we don't have to > implement/test the same ModifiedSince logic in each subclass each class might implement this differently. hard to say until we implement a few crdts, i can see this being useful for lww register, but not sure for everything (i.e. UnionSet)
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:61: VectorClock last_modified_; On 2016/10/05 16:45:10, scf wrote: > On 2016/10/05 00:57:38, steimel wrote: > > Should we make this a protected member at the SyncableMember level and then > have > > a default implementation of ModifiedSince? That way we don't have to > > implement/test the same ModifiedSince logic in each subclass > > each class might implement this differently. hard to say until we implement a > few crdts, i can see this being useful for lww register, but not sure for > everything (i.e. UnionSet) Gotcha. Makes sense. I guess in my head I figured UniqueSet would still have this last_modified_ attribute and essentially treat it as a cached max of its elements timestamps, but if we're not doing that then having having this implemented at the SyncableMember level doesn't make sense.
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable.h:81: virtual void ApplyChangeset(const VectorClock& from, I'm not very comfortable with exposing the sync and async Apply variants in the same base class. We can adapt sync to async, but not the other way around. It feels wrong to define a method in an abstract interface that is illegal for a large swath of subclasses. I recommend moving the sync Apply into SyncableMember, and the async Apply into SyncableObject, and just removing Apply from here altogether. Since this is a templated interface, it's not like we can refer to a Syncable anyways.
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable.h:81: virtual void ApplyChangeset(const VectorClock& from, I'm not very comfortable with exposing the sync and async Apply variants in the same base class. We can adapt sync to async, but not the other way around. It feels wrong to define a method in an abstract interface that is illegal for a large swath of subclasses. I recommend moving the sync Apply into SyncableMember, and the async Apply into SyncableObject, and just removing Apply from here altogether. Since this is a templated interface, it's not like we can refer to a Syncable anyways.
https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/320001/blimp/net/helium/synca... blimp/net/helium/syncable.h:140: VectorClock IncrementParentClock(); Why is there an IncrementParentClock / parent_clock still in the member? I thought the parent manages the members fully?
Updating after offline discussion with @wez and @kevin. The idea is to have the same interface for the simple and complex syncable. Removed SyncableMember. It will be just Syncable now. Complex objects will be TwoPhaseSyncable as their state is retrieved and restored in two stages. Name is still up for grabs. Let me know if you have a better name. Refactored the clock stuff out as it was causing quite some confusion and using a VectorClockGenerator now.
Updating after offline discussion with @wez and @kevin. The idea is to have the same interface for the simple and complex syncable. Removed SyncableMember. It will be just Syncable now. Complex objects will be TwoPhaseSyncable as their state is retrieved and restored in two stages. Name is still up for grabs. Let me know if you have a better name. Refactored the clock stuff out as it was causing quite some confusion and using a VectorClockGenerator now.
https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:33: // objects (i.e. Simple register) or objects that the actual truth lives or => and objects which are disconnected replicas of external state? https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:36: // Example of self contained objects are the CRDTs for example: LwwRegister, Not sure if this is necessary, a cross reference in Code Search can answer this. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: // |checkpoint|. This is a pretty important method that will remove some The "pretty important method" sentence reads too conversationally and probably belongs in a doc or in an intro comment block at the top. The interface comments should be concise and declarative. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:73: // Returns true if the object have been modified since |from|. have => has https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:77: // Extends the Syncable interface by adding support to synchronize with some synchronize => "asynchronously replicate state" ? https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:102: const base::Closure& done) = 0; You can use MandatoryCallback<void(void)> now. Or soon. Ugh, I just now realized that I need to fix the template type signature of that to match the base::Callback style... will have a CL out for that shortly. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:150: void changeChild(FakeIntSyncable* child1, Is the intention to keep these tests around, or are they just for exercising the interfaces for the development of this CL? If it's the latter then that's fine, but in the long term I think there might be too much testing of fake code to warrant unit tests. Generally speaking, interfaces and abstract classes can go without unit tests of their own. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_generator.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator.h:14: // of VectorClocks. These generators are going to be used one per host "These generators" - there's only one of them? https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator.h:17: // This greatly simplify things as VectorClocks will be behaving similar to time Paragraph seems like a design doc detail. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator.h:25: void increment() { clock_.IncrementLocal(); } capital I Increment() https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_generator_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator_unittest.cc:25: TEST_F(VectorClockGeneratorTest, nextFollowedByLast) { Don't understand this test name https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator_unittest.cc:30: TEST_F(VectorClockGeneratorTest, monotonicallyIncreasing) { MonotonicallyIncreasing
https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:74: virtual bool ModifiedSince(const VectorClock& from) = 0; Should this be const, or will there be CDRTs for which ModifiedSince will change state? https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:65: VectorClock result; Would it make more sense to use the other constructor here? i.e. have a 1-liner function: return VectorClock(remote_revision_, local_revision_);
https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:102: const base::Closure& done) = 0; On 2016/10/05 23:41:30, Kevin M wrote: > You can use MandatoryCallback<void(void)> now. Or soon. Ugh, I just now realized > that I need to fix the template type signature of that to match the > base::Callback style... will have a CL out for that shortly. OK, fixed. Now you can take a MandatoryCallback<void(void)>&&. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:111: const base::Closure& done) = 0; Use MandatoryCallback<void(void)>&&?
https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:33: // objects (i.e. Simple register) or objects that the actual truth lives On 2016/10/05 23:41:30, Kevin M wrote: > or => and > > objects which are disconnected replicas of external state? Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:36: // Example of self contained objects are the CRDTs for example: LwwRegister, On 2016/10/05 23:41:29, Kevin M wrote: > Not sure if this is necessary, a cross reference in Code Search can answer this. Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:66: // |checkpoint|. This is a pretty important method that will remove some On 2016/10/05 23:41:30, Kevin M wrote: > The "pretty important method" sentence reads too conversationally and probably > belongs in a doc or in an intro comment block at the top. The interface comments > should be concise and declarative. Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:73: // Returns true if the object have been modified since |from|. On 2016/10/05 23:41:29, Kevin M wrote: > have => has Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:74: virtual bool ModifiedSince(const VectorClock& from) = 0; On 2016/10/06 17:44:33, steimel wrote: > Should this be const, or will there be CDRTs for which ModifiedSince will change > state? good catch, should be const https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:77: // Extends the Syncable interface by adding support to synchronize with some On 2016/10/05 23:41:29, Kevin M wrote: > synchronize => "asynchronously replicate state" ? Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable.h:102: const base::Closure& done) = 0; On 2016/10/06 18:16:05, Kevin M wrote: > On 2016/10/05 23:41:30, Kevin M wrote: > > You can use MandatoryCallback<void(void)> now. Or soon. Ugh, I just now > realized > > that I need to fix the template type signature of that to match the > > base::Callback style... will have a CL out for that shortly. > > OK, fixed. Now you can take a MandatoryCallback<void(void)>&&. Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:150: void changeChild(FakeIntSyncable* child1, On 2016/10/05 23:41:30, Kevin M wrote: > Is the intention to keep these tests around, or are they just for exercising the > interfaces for the development of this CL? If it's the latter then that's fine, > but in the long term I think there might be too much testing of fake code to > warrant unit tests. Generally speaking, interfaces and abstract classes can go > without unit tests of their own. I remember @bgoldman saying he liked having the tests as a way of documenting the API. I'm fine either way. As long as we have an easy to understand Syncable I'm fine removing this. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:65: VectorClock result; On 2016/10/06 17:44:33, steimel wrote: > Would it make more sense to use the other constructor here? i.e. have a 1-liner > function: > > return VectorClock(remote_revision_, local_revision_); good catch https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_generator.h (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator.h:14: // of VectorClocks. These generators are going to be used one per host On 2016/10/05 23:41:30, Kevin M wrote: > "These generators" - there's only one of them? Done. https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator.h:17: // This greatly simplify things as VectorClocks will be behaving similar to time On 2016/10/05 23:41:30, Kevin M wrote: > Paragraph seems like a design doc detail. removed https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_generator_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/380001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_generator_unittest.cc:25: TEST_F(VectorClockGeneratorTest, nextFollowedByLast) { On 2016/10/05 23:41:30, Kevin M wrote: > Don't understand this test name Done.
The CQ bit was checked by scf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by scf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2382533002/#ps400001 (title: "addressing latest round of feedback")
The CQ bit was unchecked by scf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by scf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm % test structure nits https://codereview.chromium.org/2382533002/diff/400001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/400001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:155: void changeChild(FakeIntSyncable* child1, ChangeChild - camelCase isn't used in Cr C++ https://codereview.chromium.org/2382533002/diff/400001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:155: void changeChild(FakeIntSyncable* child1, Move output parameters to the end https://codereview.chromium.org/2382533002/diff/400001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:157: int value1_set, The purpose of these parameters is unclear from their names. https://codereview.chromium.org/2382533002/diff/420001/blimp/net/helium/synca... File blimp/net/helium/syncable_unittest.cc (right): https://codereview.chromium.org/2382533002/diff/420001/blimp/net/helium/synca... blimp/net/helium/syncable_unittest.cc:155: void changeChild(FakeIntSyncable* child1, This function does too much and has a name that's too vague - not a great helper function. Can we break this up into smaller pieces?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lethalantidote@chromium.org, steimel@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2382533002/#ps440001 (title: "test changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scf@chromium.org
The CQ bit was checked by scf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initial definition of proto messages and Syncable interface definition. BUG=650461 ========== to ========== Initial definition of proto messages and Syncable interface definition. BUG=650461 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Initial definition of proto messages and Syncable interface definition. BUG=650461 ========== to ========== Initial definition of proto messages and Syncable interface definition. BUG=650461 Committed: https://crrev.com/d10e6413f024668200243747059fcbb9ef0e00aa Cr-Commit-Position: refs/heads/master@{#423919} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/d10e6413f024668200243747059fcbb9ef0e00aa Cr-Commit-Position: refs/heads/master@{#423919} |
