Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402153002:
Add CompoundSyncable class for synchronizing containers of Syncables. (Closed)
|
Created:
4 years, 2 months ago by Kevin M Modified:
4 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CompoundSyncable class for synchronizing containers of Syncables.
* Add RegisteredMember subclass for compiler-enforced constructor
time registration of child Syncables.
* Integrate CompoundSyncable in sample unit tests.
R=scf@chromium.org,lethalantidote@chromium.org
CC=wez@chromium.org
BUG=
Committed: https://crrev.com/014a234f156e8f8002845d6d7ca2894fbaa231ac
Cr-Commit-Position: refs/heads/master@{#426874}
Patch Set 1 #
Total comments: 5
Patch Set 2 : added laggard files #
Total comments: 13
Patch Set 3 : added a registry #
Total comments: 8
Patch Set 4 : WIP patch #Patch Set 5 : much rebasery #
Total comments: 28
Patch Set 6 : scf feedback #Patch Set 7 : wez feedback #
Total comments: 15
Patch Set 8 : . #Patch Set 9 : break Syncable interface changes out into separate CL #
Total comments: 20
Patch Set 10 : wez feedback #
Total comments: 2
Patch Set 11 : LwwRegister fix #Patch Set 12 : Rebase to include RevisionGenerator. Fixed flakiness from LwwRegisterTest not cleaning up after its… #Patch Set 13 : make literal unsigned #
Messages
Total messages: 46 (14 generated)
https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/helium_err... File blimp/net/helium/helium_errors.h (right): https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:16: // Generic error if the interpreter encountered unexpected input. if the serialization reader? https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/helium_err... blimp/net/helium/helium_errors.h:17: HELIUM_ERROR(INVALID_INPUT, 3) add a prefix to tell that is related to serialization? https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/syncable.h File blimp/net/helium/syncable.h (left): https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/syncable.h... blimp/net/helium/syncable.h:85: // 1.1) PreCreateChangesetToCurrent is called which retrieves the state Don't you want to leave part of this comments? https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/syncable.h File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/syncable.h... blimp/net/helium/syncable.h:67: // Returns |true| if the changeset was applied successfully. Returns Helium::SUCCESS https://codereview.chromium.org/2402153002/diff/1/blimp/net/helium/syncable.h... blimp/net/helium/syncable.h:114: HeliumResult ApplyChangeset( new line before this https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:14: constexpr uint32_t kSentinel = 1337; i like having a preamble and postamble between the entire structure https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:38: add CHECK(changed_count > 0) ? https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:58: return HeliumResult::ERR_INVALID_INPUT; i'm wondering if those should be also CHECKS. I can only see this happening if we actually implement the serialization code incorrect, so failing fast seems the best approach https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:63: if (tag == 0 || tag > static_cast<int>(children_.size())) { add a comment that the valid ranges are 1 ... size
lgtm https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:73: // This is before calling CreateChangesetToCurrent to give a change for the change => chance?
steimel@chromium.org changed reviewers: + steimel@chromium.org
https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:38: On 2016/10/08 00:28:27, scf wrote: > add CHECK(changed_count > 0) ? But what if nothing has changed in the structure? Do we assume that CreateChangesetToCurrent is only called after checking ModifiedSince? If so, we should mention that as a precondition in the Syncable interface definition. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:69: if (child_result != HeliumResult::SUCCESS) { Should we consider having some sort of rollback procedure for children modified in previous iterations of the loop, or are we fine with partial changeset application? If there's ill-formed data in the changeset, there's reason to believe that the changes we already applied may have been corrupted
This weekend I had an idea of how to clean this up even more, will ping y'all when I land a new patch.
https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:38: On 2016/10/08 00:28:27, scf wrote: > add CHECK(changed_count > 0) ? Should be a DCHECK. CHECKs are pretty frowned upon deep in the codebase/application lifetime. If we use them, they should usually indicate unrecoverable errors near application initialization, e.g. invalid flag, corrupted config, etc. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:38: On 2016/10/10 17:31:17, steimel wrote: > On 2016/10/08 00:28:27, scf wrote: > > add CHECK(changed_count > 0) ? > > But what if nothing has changed in the structure? Do we assume that > CreateChangesetToCurrent is only called after checking ModifiedSince? If so, we > should mention that as a precondition in the Syncable interface definition. +1. We should ideally strive to avoid spurious calls to CreateChangesetToCurrent(), but if we do make them, it should just be a no-op on release builds. I'll add comments and a DCHECK to keep us honest. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:58: return HeliumResult::ERR_INVALID_INPUT; On 2016/10/08 00:28:27, scf wrote: > i'm wondering if those should be also CHECKS. I can only see this happening if > we actually implement the serialization code incorrect, so failing fast seems > the best approach There are causes other than bugs (ie. malice) that would result in us receiving mangled input. DCHECK is OK, but in release builds we can't blow up the task for network originating data. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:63: if (tag == 0 || tag > static_cast<int>(children_.size())) { On 2016/10/08 00:28:27, scf wrote: > add a comment that the valid ranges are > 1 ... size Done. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:69: if (child_result != HeliumResult::SUCCESS) { On 2016/10/10 17:31:17, steimel wrote: > Should we consider having some sort of rollback procedure for children modified > in previous iterations of the loop, or are we fine with partial changeset > application? If there's ill-formed data in the changeset, there's reason to > believe that the changes we already applied may have been corrupted If we encountered an error during changeset application, I believe the entire HeliumObject state should be considered damaged. The error path upon receiving an error to ApplyChangeset should handle the teardown, UI reporting, etc. https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/20001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:73: // This is before calling CreateChangesetToCurrent to give a change for the On 2016/10/08 00:47:13, CJ wrote: > change => chance? Done.
kmarshall@chromium.org changed reviewers: + wez@chromium.org
+wez
https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. not a huge fan of us using the protobuf api directly. specially couple of reasons: * the validation logic is intertwined * we are relying on tags of protobufs which we don't need it https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:70: uint32_t guard; either initialize to 0 or add CHECK(changeset->ReadVarint32(&guard)); https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:78: // TwoPhaseSyncable to pull the latest changes into its local state. s/TwoPhaseSyncable/Syncable/g https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:99: virtual bool ModifiedSince(const VectorClock& from) const = 0; rename to WasModifiedSince given latest design docs? https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:120: const std::vector<Syncable*>& GetMembers() const { return member_registry_; } rename to |members()| ?
https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:59: const VersionVector& from, change this to Revision? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:73: virtual void ReleaseCheckpointsBefore(const VersionVector& checkpoint) = 0; this was renamed to void ReleaseBefore(Revision& from) https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:76: virtual bool ModifiedSince(const VersionVector& from) const = 0; likewise: Revision GetRevision() const = 0;
lgtm will address those issues in a separate cl
lgtm will address those issues in a separate cl
https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. On 2016/10/11 16:53:56, scf wrote: > not a huge fan of us using the protobuf api directly. specially couple of > reasons: > * the validation logic is intertwined > * we are relying on tags of protobufs which we don't need it * Validation: that's fair, fixed. * "Tags" are not protobuf types per se, they are just varint readers/writers that use return values instead of output parameters. https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:70: uint32_t guard; On 2016/10/11 16:53:57, scf wrote: > either initialize to 0 or add CHECK(changeset->ReadVarint32(&guard)); Added a DCHECK to the read function. FYI, we should never CHECK on network originating inputs. Use of CHECK is generally considered an antipattern except in certain cases like unrecoverable initialization failures. https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:120: const std::vector<Syncable*>& GetMembers() const { return member_registry_; } On 2016/10/11 16:53:57, scf wrote: > rename to |members()| ? Done.
https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. On 2016/10/11 16:53:56, scf wrote: > not a huge fan of us using the protobuf api directly. specially couple of > reasons: > * the validation logic is intertwined > * we are relying on tags of protobufs which we don't need it * Validation: that's fair, fixed. * "Tags" are not protobuf types per se, they are just varint readers/writers that use return values instead of output parameters. https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:70: uint32_t guard; On 2016/10/11 16:53:57, scf wrote: > either initialize to 0 or add CHECK(changeset->ReadVarint32(&guard)); Added a DCHECK to the read function. FYI, we should never CHECK on network originating inputs. Use of CHECK is generally considered an antipattern except in certain cases like unrecoverable initialization failures. https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/40001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:120: const std::vector<Syncable*>& GetMembers() const { return member_registry_; } On 2016/10/11 16:53:57, scf wrote: > rename to |members()| ? Done.
Some initial comments :) https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:35: // using a bit array stored in a string. ot-nit: Ick. Better to use a varint and limit us to fan-out of 64 syncables per composed object, perhaps. :P https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:43: #if DCHECK_IS_ON() Downside of using DCHECK_IS_ON() here is that we can't have Release clients connect to a Debug engine, for example, any more. Perhaps make that a startup parameter instead? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. Tags are protobuf-internal; we should be explicit that we're just writing out a varint here. Protobuf uses 1-based index values so that ReadTag() can return zero in case of ReadVarint32 returning false (failure), IIUC; I don't think we need to replicate that behaviour, do we? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:89: return HeliumResult::ERR_TAG_OUT_OF_RANGE; This seems too specific an error; the peer basically sent invalid protocol data, so perhaps just have an ERR_INVALID_PROTOCOL error. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:69: google::protobuf::io::CodedInputStream* input_stream) = 0; As discussed offline, we only strictly need the remote-revision of the |to| VectorClock as an explicit parameter. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:76: virtual bool ModifiedSince(const VersionVector& from) const = 0; On 2016/10/17 20:04:08, scf wrote: > likewise: > > Revision GetRevision() const = 0; As discussed offline, this should be: VersionVector GetVersionVector() const = 0; for example. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:118: class RegisteredMember; Do we need this and the Structure in this header? Feels cleaner to pull them out to their own, since they are concrete implementations, albeit with templates, whereas the preceding interfaces are pure. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:121: class MemberRegistry { nit: Does it significantly simplify SyncableStructure to have this as a separate class? Or could we tuck it away inside the impl? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:148: ~RegisteredMember() = default; Doesn't this mean that a caller could (incorrectly) std::move() a RegisteredMember into another, or just delete it, and end up with a registered-but-deleted member? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:150: T* get() const { return member_.get(); } nit: what about operator->()? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:168: // member_registry()->Create<SyncableType>(...) . nit: Wrap that verbosity in a CreateMember helper? https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:169: class SyncableStructure : public Syncable { nit: Suggest SyncableStruct or ComposedSyncable or CompoundSyncable.
https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. On 2016/10/18 01:39:11, Wez wrote: > Tags are protobuf-internal; we should be explicit that we're just writing out a > varint here. > > Protobuf uses 1-based index values so that ReadTag() can return zero in case of > ReadVarint32 returning false (failure), IIUC; I don't think we need to replicate > that behaviour, do we? Agree here, this was one of my earlier comments. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:89: return HeliumResult::ERR_TAG_OUT_OF_RANGE; On 2016/10/18 01:39:11, Wez wrote: > This seems too specific an error; the peer basically sent invalid protocol data, > so perhaps just have an ERR_INVALID_PROTOCOL error. I like being specific here, having a generic error might induce future changes uses the same which makes debugging a nightmare. Maybe calling ERR_CANNOT_READ_FIELD_ID ?
https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.cc (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:35: // using a bit array stored in a string. On 2016/10/18 01:39:11, Wez wrote: > ot-nit: Ick. Better to use a varint and limit us to fan-out of 64 syncables per > composed object, perhaps. :P Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:43: #if DCHECK_IS_ON() On 2016/10/18 01:39:11, Wez wrote: > Downside of using DCHECK_IS_ON() here is that we can't have Release clients > connect to a Debug engine, for example, any more. > > Perhaps make that a startup parameter instead? Eh... this was useful during development, but I'm not sure if the sentinel values are worth keeping around after we land this due to the extreme simplicity of the structure. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:50: changeset->WriteTag(i + 1); // PB tags are 1-indexed. On 2016/10/18 01:39:11, Wez wrote: > Tags are protobuf-internal; we should be explicit that we're just writing out a > varint here. > > Protobuf uses 1-based index values so that ReadTag() can return zero in case of > ReadVarint32 returning false (failure), IIUC; I don't think we need to replicate > that behaviour, do we? Switched to encoding a varint bitset instead - it's actually *less* error prone because there are fewer values to go wrong (duplicate tag ids, invalid tag ids, bad count, etc.) https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.cc:89: return HeliumResult::ERR_TAG_OUT_OF_RANGE; On 2016/10/18 16:48:28, scf wrote: > On 2016/10/18 01:39:11, Wez wrote: > > This seems too specific an error; the peer basically sent invalid protocol > data, > > so perhaps just have an ERR_INVALID_PROTOCOL error. > > I like being specific here, having a generic error might induce future changes > uses the same which makes debugging a nightmare. Maybe calling > ERR_CANNOT_READ_FIELD_ID ? I tend to agree with Sergio. If we own the error space, why not err on the side of high granularity to make life easier on ourselves? Anyways, this specific issue is N/A because field IDs won't be specified anymore. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:59: const VersionVector& from, On 2016/10/17 20:04:08, scf wrote: > change this to Revision? Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:69: google::protobuf::io::CodedInputStream* input_stream) = 0; On 2016/10/18 01:39:11, Wez wrote: > As discussed offline, we only strictly need the remote-revision of the |to| > VectorClock as an explicit parameter. Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:73: virtual void ReleaseCheckpointsBefore(const VersionVector& checkpoint) = 0; On 2016/10/17 20:04:08, scf wrote: > this was renamed to > > void ReleaseBefore(Revision& from) Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:118: class RegisteredMember; On 2016/10/18 01:39:11, Wez wrote: > Do we need this and the Structure in this header? Feels cleaner to pull them out > to their own, since they are concrete implementations, albeit with templates, > whereas the preceding interfaces are pure. Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:121: class MemberRegistry { On 2016/10/18 01:39:11, Wez wrote: > nit: Does it significantly simplify SyncableStructure to have this as a separate > class? Or could we tuck it away inside the impl? Merged with impl. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:168: // member_registry()->Create<SyncableType>(...) . On 2016/10/18 01:39:11, Wez wrote: > nit: Wrap that verbosity in a CreateMember helper? Done. https://codereview.chromium.org/2402153002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable.h:169: class SyncableStructure : public Syncable { On 2016/10/18 01:39:11, Wez wrote: > nit: Suggest SyncableStruct or ComposedSyncable or CompoundSyncable. Done.
https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:39: int prev_bytes_written = changeset->ByteCount(); don't you have to have this under the |DEBUG| symbol to avoid the un-used variable error on release build? https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... File blimp/helium/compound_syncable.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... blimp/helium/compound_syncable.h:61: RegisteredMember(RegisteredMember<T>&& other) {} woudn't disable be RegisteredMember(RegisteredMember<T>&& other) = delete; instead? https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/errors.h File blimp/helium/errors.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/errors.h#... blimp/helium/errors.h:24: HELIUM_ERROR(STRUCTURE_HEADER_READ_ERROR, 4) nit: move this to line 18? https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.h File blimp/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:68: // concurrent change conflicts. update the comment. i think this revision is also the remote one right? i would call something that would convey that too. i.e. |to_remote| https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:74: // |checkpoint|. update to |from| https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:97: class TwoPhaseSyncable : public Syncable { i'm a bit confused we have the compoundsyncable and twophasesyncable. we are removing this right? and merging into the syncable interface? https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:106: virtual void PreCreateChangesetToCurrent(const VersionVector& from, change to Revision from https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:114: virtual void PostApplyChangeset(const VersionVector& from, we can remove this based on latest discussions right?
https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:39: int prev_bytes_written = changeset->ByteCount(); On 2016/10/18 22:08:47, scf wrote: > don't you have to have this under the |DEBUG| symbol to avoid the un-used > variable error on release build? Probably! Fixed. https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... File blimp/helium/compound_syncable.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/compound_... blimp/helium/compound_syncable.h:61: RegisteredMember(RegisteredMember<T>&& other) {} On 2016/10/18 22:08:47, scf wrote: > woudn't disable be > > RegisteredMember(RegisteredMember<T>&& other) = delete; > > instead? Neat! C++11! https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/errors.h File blimp/helium/errors.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/errors.h#... blimp/helium/errors.h:24: HELIUM_ERROR(STRUCTURE_HEADER_READ_ERROR, 4) On 2016/10/18 22:08:47, scf wrote: > nit: move this to line 18? Done. https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.h File blimp/helium/syncable.h (right): https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:74: // |checkpoint|. On 2016/10/18 22:08:47, scf wrote: > update to |from| Done. https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:97: class TwoPhaseSyncable : public Syncable { On 2016/10/18 22:08:47, scf wrote: > i'm a bit confused we have the compoundsyncable and twophasesyncable. we are > removing this right? and merging into the syncable interface? No, I thought we were keeping the TwoPhase specialization instead of adding virtual methods to Syncable that are N/A for all but the top level state objects. https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:106: virtual void PreCreateChangesetToCurrent(const VersionVector& from, On 2016/10/18 22:08:47, scf wrote: > change to Revision from Done. https://codereview.chromium.org/2402153002/diff/120001/blimp/helium/syncable.... blimp/helium/syncable.h:114: virtual void PostApplyChangeset(const VersionVector& from, On 2016/10/18 22:08:47, scf wrote: > we can remove this based on latest discussions right? Done.
lgtm
lgtm
https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:26: // The bits for unmodified members_ are left as zero. I thought we had agreed to stick with the <count>(<field-id><payload>)* format rather than optimizing to bit-array, initially? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:28: std::bitset<64> modified; Is there any real advantage to using bitset here, rather than just doing the bit-twiddling into a uint64_t manually? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:46: DCHECK_GT(changeset->ByteCount(), prev_bytes_written); Doesn't DCHECK_OP() still _reference_ all parameters, even in non-DCHECK builds? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:61: for (size_t member_id = 0u; member_id < members_.size(); ++member_id) { nit: Is there an C++11 iteration for bitset? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:77: for (const auto& next_child : members_) { nit: Why |next_child| rather than |member|, or perhaps |syncable|? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... File blimp/helium/compound_syncable.h (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.h:30: // CreateAndRegister(<ctor args>). Looks like we have two class descriptions? Can we split the description into two parts: (1) what it is for (i.e. composing sets of Syncables together into a single Syncable) and (2) how it is used? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.h:50: ~RegisteredMember() = default; nit: destructor comes after constructor(s) https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h File blimp/helium/errors.h (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h#... blimp/helium/errors.h:10: // A generic error occurred. nit: Why not call it GENERIC_ERROR, or UNKNOWN_ERROR, then? Where do we actually use this, though? Why do we even have an "INTERNAL_ERROR" code? https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h#... blimp/helium/errors.h:21: HELIUM_ERROR(STRUCTURE_HEADER_READ_ERROR, 4) Given that we have a DCHECK for debug builds, is there value in this extra error code over PROTOCOL_ERROR? It's effectively just a sub-type of PROTOCOL_ERROR, after all.
https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:26: // The bits for unmodified members_ are left as zero. On 2016/10/19 22:54:24, Wez wrote: > I thought we had agreed to stick with the <count>(<field-id><payload>)* format > rather than optimizing to bit-array, initially? I discovered that using a bitarray was actually *easier* in some ways since it's more constrained of input than a sequence of arbitrary IDs. It's impossible for fields to be specified more than once, for example. https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:28: std::bitset<64> modified; On 2016/10/19 22:54:25, Wez wrote: > Is there any real advantage to using bitset here, rather than just doing the > bit-twiddling into a uint64_t manually? Readability? I'm happy doing bit-twiddling too, but this seems like a nice wrapper around shifts 'n masks. https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:46: DCHECK_GT(changeset->ByteCount(), prev_bytes_written); On 2016/10/19 22:54:24, Wez wrote: > Doesn't DCHECK_OP() still _reference_ all parameters, even in non-DCHECK builds? Ah right, it does - it just never gets evaluated b/c of the ternary operator check w/DCHECK_IS_ON. OK, this is fine https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:61: for (size_t member_id = 0u; member_id < members_.size(); ++member_id) { On 2016/10/19 22:54:24, Wez wrote: > nit: Is there an C++11 iteration for bitset? Nein https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:77: for (const auto& next_child : members_) { On 2016/10/19 22:54:25, Wez wrote: > nit: Why |next_child| rather than |member|, or perhaps |syncable|? Done. https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h File blimp/helium/errors.h (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h#... blimp/helium/errors.h:10: // A generic error occurred. On 2016/10/19 22:54:25, Wez wrote: > nit: Why not call it GENERIC_ERROR, or UNKNOWN_ERROR, then? > > Where do we actually use this, though? Why do we even have an "INTERNAL_ERROR" > code? Garrett specified this in the transport doc - but probably generic error codes can be qualified with the component that produced them (e.g. GRPC_UNKNOWN_ERROR) https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/errors.h#... blimp/helium/errors.h:21: HELIUM_ERROR(STRUCTURE_HEADER_READ_ERROR, 4) On 2016/10/19 22:54:25, Wez wrote: > Given that we have a DCHECK for debug builds, is there value in this extra error > code over PROTOCOL_ERROR? It's effectively just a sub-type of PROTOCOL_ERROR, > after all. Done.
lgtm https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:26: // The bits for unmodified members_ are left as zero. On 2016/10/19 23:22:44, Kevin M wrote: > On 2016/10/19 22:54:24, Wez wrote: > > I thought we had agreed to stick with the <count>(<field-id><payload>)* format > > rather than optimizing to bit-array, initially? > > I discovered that using a bitarray was actually *easier* in some ways since it's > more constrained of input than a sequence of arbitrary IDs. It's impossible for > fields to be specified more than once, for example. That seems like a constraint that's easy enough to enforce at the receiver, but fair enough. https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:46: DCHECK_GT(changeset->ByteCount(), prev_bytes_written); On 2016/10/19 23:22:44, Kevin M wrote: > On 2016/10/19 22:54:24, Wez wrote: > > Doesn't DCHECK_OP() still _reference_ all parameters, even in non-DCHECK > builds? > > Ah right, it does - it just never gets evaluated b/c of the ternary operator > check w/DCHECK_IS_ON. OK, this is fine Doesn't that mean that the compiler complains if DCHECK_IS_ON is not set, though, since although it'll never actually evaluate prev_bytes_written, it _is_ referenced, but won't be defined? https://codereview.chromium.org/2402153002/diff/180001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/180001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:61: "of registered fields."; nit: Do we really need the explanatory text here & above; this will only fire in DCHECK/debug builds, and will log the line at which the failure occurs, which should be sufficient to debug.
Description was changed from ========== Add SyncableStructure class for synchronizing containers of Syncables. * Add SyncableStructure class for synchronizing containers of Syncables. * Move TwoPhaseSyncable calls into Syncable, to keep the Syncable interface contract consistent at the aggregation layers. * Add SynchronizedChild subclass for compiler-enforced constructor time registration of child Syncables. * Integrate SyncableStructure in sample unit tests. * Renamed "fakes" to "samples" in unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ========== to ========== Add SyncableStructure class for synchronizing containers of Syncables. * Add SyncableStructure class for synchronizing containers of Syncables. * Move TwoPhaseSyncable calls into Syncable, to keep the Syncable interface contract consistent at the aggregation layers. * Add SynchronizedChild subclass for compiler-enforced constructor time registration of child Syncables. * Integrate SyncableStructure in sample unit tests. * Renamed "fakes" to "samples" in unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ==========
Description was changed from ========== Add SyncableStructure class for synchronizing containers of Syncables. * Add SyncableStructure class for synchronizing containers of Syncables. * Move TwoPhaseSyncable calls into Syncable, to keep the Syncable interface contract consistent at the aggregation layers. * Add SynchronizedChild subclass for compiler-enforced constructor time registration of child Syncables. * Integrate SyncableStructure in sample unit tests. * Renamed "fakes" to "samples" in unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ========== to ========== Add CompoundSyncable class for synchronizing containers of Syncables. * Add RegisteredMember subclass for compiler-enforced constructor time registration of child Syncables. * Integrate CompoundSyncable in sample unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ==========
https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:26: // The bits for unmodified members_ are left as zero. On 2016/10/20 21:22:31, Wez wrote: > On 2016/10/19 23:22:44, Kevin M wrote: > > On 2016/10/19 22:54:24, Wez wrote: > > > I thought we had agreed to stick with the <count>(<field-id><payload>)* > format > > > rather than optimizing to bit-array, initially? > > > > I discovered that using a bitarray was actually *easier* in some ways since > it's > > more constrained of input than a sequence of arbitrary IDs. It's impossible > for > > fields to be specified more than once, for example. > > That seems like a constraint that's easy enough to enforce at the receiver, but > fair enough. It's pretty trivial to do, but the tag number approach just seems worse all around - it adds more LoC and bloats up the wire representation. https://codereview.chromium.org/2402153002/diff/160001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:46: DCHECK_GT(changeset->ByteCount(), prev_bytes_written); On 2016/10/20 21:22:31, Wez wrote: > On 2016/10/19 23:22:44, Kevin M wrote: > > On 2016/10/19 22:54:24, Wez wrote: > > > Doesn't DCHECK_OP() still _reference_ all parameters, even in non-DCHECK > > builds? > > > > Ah right, it does - it just never gets evaluated b/c of the ternary operator > > check w/DCHECK_IS_ON. OK, this is fine > > Doesn't that mean that the compiler complains if DCHECK_IS_ON is not set, > though, since although it'll never actually evaluate prev_bytes_written, it _is_ > referenced, but won't be defined? Done. https://codereview.chromium.org/2402153002/diff/180001/blimp/helium/compound_... File blimp/helium/compound_syncable.cc (right): https://codereview.chromium.org/2402153002/diff/180001/blimp/helium/compound_... blimp/helium/compound_syncable.cc:61: "of registered fields."; On 2016/10/20 21:22:32, Wez wrote: > nit: Do we really need the explanatory text here & above; this will only fire in > DCHECK/debug builds, and will log the line at which the failure occurs, which > should be sufficient to debug. Done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lethalantidote@chromium.org, scf@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2402153002/#ps200001 (title: "LwwRegister fix")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Made some misc. fixes to LwwRegister and flakiness due to RevisionGenerator global state not being cleaned up.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scf@chromium.org, wez@chromium.org, lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2402153002/#ps220001 (title: "Rebase to include RevisionGenerator. Fixed flakiness from LwwRegisterTest not cleaning up after its…")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scf@chromium.org, wez@chromium.org, lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2402153002/#ps240001 (title: "make literal unsigned")
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 ========== Add CompoundSyncable class for synchronizing containers of Syncables. * Add RegisteredMember subclass for compiler-enforced constructor time registration of child Syncables. * Integrate CompoundSyncable in sample unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ========== to ========== Add CompoundSyncable class for synchronizing containers of Syncables. * Add RegisteredMember subclass for compiler-enforced constructor time registration of child Syncables. * Integrate CompoundSyncable in sample unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add CompoundSyncable class for synchronizing containers of Syncables. * Add RegisteredMember subclass for compiler-enforced constructor time registration of child Syncables. * Integrate CompoundSyncable in sample unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= ========== to ========== Add CompoundSyncable class for synchronizing containers of Syncables. * Add RegisteredMember subclass for compiler-enforced constructor time registration of child Syncables. * Integrate CompoundSyncable in sample unit tests. R=scf@chromium.org,lethalantidote@chromium.org CC=wez@chromium.org BUG= Committed: https://crrev.com/014a234f156e8f8002845d6d7ca2894fbaa231ac Cr-Commit-Position: refs/heads/master@{#426874} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/014a234f156e8f8002845d6d7ca2894fbaa231ac Cr-Commit-Position: refs/heads/master@{#426874} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
