Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400303002:
Add LwwRegister CRDT (Closed)
|
Created:
4 years, 2 months ago by steimel 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 LwwRegister CRDT
BUG=650466
Committed: https://crrev.com/cc4584d7c74a937b617180bc69a5816b49f004b3
Cr-Commit-Position: refs/heads/master@{#426549}
Patch Set 1 #Patch Set 2 : Add LwwRegister and tests to build #
Total comments: 43
Patch Set 3 : Update from VectorClock to VersionVector. Refactor unit tests. Code review comments. #Patch Set 4 : Update LwwRegister to use Coded(In/Out)putStream. LwwRegisterSerializer -> SyncablePrimitiveSeriali… #
Total comments: 35
Patch Set 5 : Refactor Serializer to use overloaded methods and add unit tests. Refactor LwwRegister to not be te… #
Total comments: 27
Patch Set 6 : Refactor SyncablePrimitiveSerializer tests and other code review feedback. #
Total comments: 18
Patch Set 7 : Combine Bias and RunningAs enums, plus other code review feedback. #
Total comments: 25
Patch Set 8 : Rebase with Syncable interface changes + code review feedback #
Total comments: 10
Patch Set 9 : SyncablePrimitiveSerializer->CodedValueSerializer #
Total comments: 2
Patch Set 10 : Add comment on max string length constant #
Messages
Total messages: 38 (5 generated)
lethalantidote@chromium.org changed reviewers: + lethalantidote@chromium.org
https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:25: explicit LwwRegister(VectorClockGenerator* clock_gen, RunningAs running_as); Is there a particular reason why this has be to be explicit? AFAIK, it only needs to be such if it is a single parameter constructor.
steimel@chromium.org changed reviewers: + kmarshall@chromium.org, scf@chromium.org
PTAL. Still need to refactor tests, but other than that it's functional https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:25: explicit LwwRegister(VectorClockGenerator* clock_gen, RunningAs running_as); On 2016/10/07 22:07:54, CJ wrote: > Is there a particular reason why this has be to be explicit? AFAIK, it only > needs to be such if it is a single parameter constructor. You actually can implicitly construct with multiple parameters (http://en.cppreference.com/w/cpp/language/explicit), though it's not super common afaik
https://codereview.chromium.org/2400303002/diff/20001/blimp/common/proto/heli... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/common/proto/heli... blimp/common/proto/helium.proto:30: int32 integer_value = 1; rename to something that convey the size: |i32_value| https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:18: enum class RunningAs { RunningAsClient, RunningAsEngine }; This should probably go into its own file as its going to be reused elsewhere https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:19: enum class LwwBias { ClientWins, EngineWins }; move this inside LwwRegister https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:28: void set(const RegisterType& state); rename to |Set| as its a "complex" operation https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:30: const RegisterType& state() const; rename to get() https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:46: RegisterType state_; provably rename to value_ https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:61: LwwRegister<RegisterType, bias>::~LwwRegister() = default; dumb question, does the compiler complains this is complex? Otherwise could move this to the declaration https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:97: switch (last_modified_.CompareTo(to)) { I find this a bit difficult to read. ``` if (dont_need_to_deserialize) return; deserialize(); ``` Its a bit more readable IMO, it also removes the empty case statements. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_serializer.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_serializer.h:28: template <class RegisterType> I prefer removing this code so you get a compiler error instead of runtime error. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:49: }; It would be great if the test could be written a bit more declarative and with the help of some more helper functions. For example: ``` auto& client = getClient(ClientWins); auto& engine = getEngine(ClientWins); client.Set(123); SyncFromClient(); EXPECT_EQ(123, engine.value()); ``` Ideally you would feed ClientWins or EngineWins and reuse the same body, except the outcome of the test that could be different. Maybe focus on 1-3 tests checking for ModifiedSince, but the remainder would be focused on Set/Get/Creating and applying changesets. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:56: EXPECT_EQ(current_local_revision + 1, local_gen_->current().local_revision()); you probably don't want to make assumptions wrt counter being just + 1 with the local value. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:59: TEST_F(LwwRegisterTest, ModifiedSinceWorks) { probably want to merge with the previous test. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:64: later_revision.IncrementLocal(); i dont think you need this, if its greater than earlier its probably good enough https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:89: lww_register_->ApplyChangeset(from, to, std::move(changeset)); probably want to encapsulate this to something like: Sync(LwwRegister src, LwwRegister target)
https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:18: enum class RunningAs { RunningAsClient, RunningAsEngine }; Could simplify the enums as just "Client" or "Engine", since it would come up as RunningAs::Client etc.
Heads up: the change to using byte output streams instead of proto messages will undoubtedly affect portions of this CL. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:18: enum class RunningAs { RunningAsClient, RunningAsEngine }; On 2016/10/10 20:25:55, CJ wrote: > Could simplify the enums as just "Client" or "Engine", since it would come up as > RunningAs::Client etc. +1. "enum class" is nice for enforcing prefixes. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:19: enum class LwwBias { ClientWins, EngineWins }; On 2016/10/10 19:45:47, scf wrote: > move this inside LwwRegister Could this just be factored out as "Bias"? https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:21: template <class RegisterType, LwwBias bias> Add comments https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:25: explicit LwwRegister(VectorClockGenerator* clock_gen, RunningAs running_as); On 2016/10/07 22:53:39, steimel wrote: > On 2016/10/07 22:07:54, CJ wrote: > > Is there a particular reason why this has be to be explicit? AFAIK, it only > > needs to be such if it is a single parameter constructor. > > You actually can implicitly construct with multiple parameters > (http://en.cppreference.com/w/cpp/language/explicit), though it's not super > common afaik What are the cases in which a two-param constructor can implicitly be called? The style convention (not a hard rule) is to only use explicit for single param. I recommend not using explicit otherwise, because readers might mistake "explicit" for "unary" when skimming. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:127: template <class RegisterType, LwwBias bias> Move this to wherever it is that we will be stashing Bias and OwnedBy? https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_serializer.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_serializer.h:28: template <class RegisterType> On 2016/10/10 19:45:47, scf wrote: > I prefer removing this code so you get a compiler error instead of runtime > error. +1 https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_serializer.h:44: void LwwRegisterSerializer<int32_t>::SerializeLwwRegister( Since we'll be serializing primitive data types all the time, I recommend moving these functions into a common library which, given a value and a CodedOutputStream, will the value to the stream using the appropriate Write method.
https://codereview.chromium.org/2400303002/diff/20001/blimp/common/proto/heli... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/common/proto/heli... blimp/common/proto/helium.proto:30: int32 integer_value = 1; On 2016/10/10 19:45:46, scf wrote: > rename to something that convey the size: |i32_value| Gotcha. If we end up keeping with protos instead of moving to CodedInputStreams, I'll update the name https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:18: enum class RunningAs { RunningAsClient, RunningAsEngine }; On 2016/10/10 19:45:46, scf wrote: > This should probably go into its own file as its going to be reused elsewhere Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:19: enum class LwwBias { ClientWins, EngineWins }; On 2016/10/10 19:45:47, scf wrote: > move this inside LwwRegister LwwRegister is a templated class that is templated based on this enum. AFAIK, having something templated by something that is inside the template is not going to work out. I separated this out in the same way as RunningAs since it may be useful elsewhere (e.g. OwnedRegister) https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:21: template <class RegisterType, LwwBias bias> On 2016/10/10 20:50:21, Kevin M wrote: > Add comments Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:21: template <class RegisterType, LwwBias bias> On 2016/10/10 20:50:21, Kevin M wrote: > Add comments Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:25: explicit LwwRegister(VectorClockGenerator* clock_gen, RunningAs running_as); On 2016/10/10 20:50:21, Kevin M wrote: > On 2016/10/07 22:53:39, steimel wrote: > > On 2016/10/07 22:07:54, CJ wrote: > > > Is there a particular reason why this has be to be explicit? AFAIK, it only > > > needs to be such if it is a single parameter constructor. > > > > You actually can implicitly construct with multiple parameters > > (http://en.cppreference.com/w/cpp/language/explicit), though it's not super > > common afaik > > What are the cases in which a two-param constructor can implicitly be called? > > The style convention (not a hard rule) is to only use explicit for single param. > I recommend not using explicit otherwise, because readers might mistake > "explicit" for "unary" when skimming. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:28: void set(const RegisterType& state); On 2016/10/10 19:45:47, scf wrote: > rename to |Set| as its a "complex" operation Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:30: const RegisterType& state() const; On 2016/10/10 19:45:46, scf wrote: > rename to get() Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:46: RegisterType state_; On 2016/10/10 19:45:47, scf wrote: > provably rename to value_ Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:61: LwwRegister<RegisterType, bias>::~LwwRegister() = default; On 2016/10/10 19:45:46, scf wrote: > dumb question, does the compiler complains this is complex? Otherwise could move > this to the declaration Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:97: switch (last_modified_.CompareTo(to)) { On 2016/10/10 19:45:47, scf wrote: > I find this a bit difficult to read. > > ``` > if (dont_need_to_deserialize) return; > > deserialize(); > ``` > > Its a bit more readable IMO, it also removes the empty case statements. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:127: template <class RegisterType, LwwBias bias> On 2016/10/10 20:50:21, Kevin M wrote: > Move this to wherever it is that we will be stashing Bias and OwnedBy? This particular function needs to know about internal object state (bias and running_as_). Do you want a function that will intake a Bias and a RunningAs and output a bool that all Biased CRDTs can call? https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_serializer.h (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_serializer.h:28: template <class RegisterType> On 2016/10/10 19:45:47, scf wrote: > I prefer removing this code so you get a compiler error instead of runtime > error. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_serializer.h:44: void LwwRegisterSerializer<int32_t>::SerializeLwwRegister( On 2016/10/10 20:50:21, Kevin M wrote: > Since we'll be serializing primitive data types all the time, I recommend moving > these functions into a common library which, given a value and a > CodedOutputStream, will the value to the stream using the appropriate Write > method. Gotcha. I'll do that when I make the change to CodedOutputStreams https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:49: }; On 2016/10/10 19:45:47, scf wrote: > It would be great if the test could be written a bit more declarative and with > the help of some more helper functions. > > For example: > > ``` > auto& client = getClient(ClientWins); > auto& engine = getEngine(ClientWins); > > client.Set(123); > > SyncFromClient(); > > EXPECT_EQ(123, engine.value()); > ``` > > Ideally you would feed ClientWins or EngineWins and reuse the same body, except > the outcome of the test that could be different. > > Maybe focus on 1-3 tests checking for ModifiedSince, but the remainder would be > focused on Set/Get/Creating and applying changesets. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:56: EXPECT_EQ(current_local_revision + 1, local_gen_->current().local_revision()); On 2016/10/10 19:45:47, scf wrote: > you probably don't want to make assumptions wrt counter being just + 1 with the > local value. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:59: TEST_F(LwwRegisterTest, ModifiedSinceWorks) { On 2016/10/10 19:45:47, scf wrote: > probably want to merge with the previous test. Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:64: later_revision.IncrementLocal(); On 2016/10/10 19:45:47, scf wrote: > i dont think you need this, if its greater than earlier its probably good enough Done. https://codereview.chromium.org/2400303002/diff/20001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:89: lww_register_->ApplyChangeset(from, to, std::move(changeset)); On 2016/10/10 19:45:47, scf wrote: > probably want to encapsulate this to something like: > > Sync(LwwRegister src, LwwRegister target) Done.
https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:33: void SyncablePrimitiveSerializer<int32_t>::SerializeSyncablePrimitive( Pretty sure we could simplify these function names to just Serialize/Deserialize, since the class name is so descriptive.
https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:44: .ReadVarint32((uint32_t*)&value); add check? https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:61: coded_input_stream.ReadVarint32(&length); add check
https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h File blimp/net/helium/bias.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h... blimp/net/helium/bias.h:5: #ifndef BLIMP_NET_HELIUM_BIAS_H We'll need a home for RunningAs, too. How about syncable_common.h? https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:29: void Set(const RegisterType& value); "set_value()" would also work. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:31: const RegisterType& get() const; "Get()", or "value()". https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:110: bool LwwRegister<RegisterType, bias>::LocalWinsConflict() const { Two cases = this should probably be an if/elseif. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:16: class LwwRegisterTest : public testing::Test { This has too many helper functions :\ https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:36: std::unique_ptr<VersionVectorGenerator> local_gen_; Why are these unique ptrs instead of just declared directly in the class? https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:36: std::unique_ptr<VersionVectorGenerator> local_gen_; "Local" and "remote"? Can we be consistent with "client" and "engine" terminology? https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:49: LwwRegister<int, bias>* getLwwRegister(RunningAs running_as); Why don't we just declare protected LwwRegister fields |client_register_| and |engine_register_| instead? We can get rid of these templated methods that way. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:61: VersionVectorGenerator* getVersionGenerator(RunningAs running_as) { Helper fn isn't necessary; just have test code call local_gen_/remote_gen_ directly. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:70: template <Bias bias> This can be simplified as template <typename FromRegisterType, ToRegisterType> Sync(FromRegisterType* from_lww..., ToRegisterType* to_lww..., VersionVector etc etc) which doesn't need helper functions to be useful. So you'd write Sync(&engine_register_, &client_register_, &engine_gen_, &client_gen); https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:19: template <class RegisterType> Recommend making these standalone templated functions instead of popping them in specialized templated classes, so that we get automatic template inference superpowers. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:25: static void DeserializeSyncablePrimitive( Should this return a bool? https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:26: RegisterType& value, Output parameters should be pointers and located at the end of the parameter list. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:51: google::protobuf::io::CodedOutputStream coded_output_stream(output_stream); These should take CodedStreams as parameters (there is a pending CL which fixes this that is in comment limbo ATM, urgh)
https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:110: bool LwwRegister<RegisterType, bias>::LocalWinsConflict() const { Can we just precompute this in the constructor, e.g. storing it as "locally_owned_" or something? Also, is there any advantage to having this class be templated w/Bias?
https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can you add unit tests for every supported datatype here?
OK, you can rebase on master now - the Coded*Stream CL has landed.
PTAL https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h File blimp/net/helium/bias.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h... blimp/net/helium/bias.h:5: #ifndef BLIMP_NET_HELIUM_BIAS_H On 2016/10/13 22:03:36, Kevin M wrote: > We'll need a home for RunningAs, too. How about syncable_common.h? Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:29: void Set(const RegisterType& value); On 2016/10/13 22:03:36, Kevin M wrote: > "set_value()" would also work. Acknowledged. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:31: const RegisterType& get() const; On 2016/10/13 22:03:36, Kevin M wrote: > "Get()", or "value()". Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:110: bool LwwRegister<RegisterType, bias>::LocalWinsConflict() const { On 2016/10/13 22:03:36, Kevin M wrote: > Two cases = this should probably be an if/elseif. Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:16: class LwwRegisterTest : public testing::Test { On 2016/10/13 22:03:36, Kevin M wrote: > This has too many helper functions :\ Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:36: std::unique_ptr<VersionVectorGenerator> local_gen_; On 2016/10/13 22:03:36, Kevin M wrote: > Why are these unique ptrs instead of just declared directly in the class? Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:49: LwwRegister<int, bias>* getLwwRegister(RunningAs running_as); On 2016/10/13 22:03:36, Kevin M wrote: > Why don't we just declare protected LwwRegister fields |client_register_| and > |engine_register_| instead? We can get rid of these templated methods that way. Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:61: VersionVectorGenerator* getVersionGenerator(RunningAs running_as) { On 2016/10/13 22:03:36, Kevin M wrote: > Helper fn isn't necessary; just have test code call local_gen_/remote_gen_ > directly. Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/14 17:12:35, Kevin M wrote: > Can you add unit tests for every supported datatype here? Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:19: template <class RegisterType> On 2016/10/13 22:03:36, Kevin M wrote: > Recommend making these standalone templated functions instead of popping them in > specialized templated classes, so that we get automatic template inference > superpowers. Discussed offline. Changing to overloaded instead. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:25: static void DeserializeSyncablePrimitive( On 2016/10/13 22:03:36, Kevin M wrote: > Should this return a bool? Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:26: RegisterType& value, On 2016/10/13 22:03:36, Kevin M wrote: > Output parameters should be pointers and located at the end of the parameter > list. Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:33: void SyncablePrimitiveSerializer<int32_t>::SerializeSyncablePrimitive( On 2016/10/12 22:32:44, CJ wrote: > Pretty sure we could simplify these function names to just > Serialize/Deserialize, since the class name is so descriptive. Done. https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:44: .ReadVarint32((uint32_t*)&value); On 2016/10/12 23:05:12, scf wrote: > add check? Discussed with Kevin offline. Returning bool instead https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.h:51: google::protobuf::io::CodedOutputStream coded_output_stream(output_stream); On 2016/10/13 22:03:36, Kevin M wrote: > These should take CodedStreams as parameters (there is a pending CL which fixes > this that is in comment limbo ATM, urgh) Done.
just a couple of nits but lgtm overall https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h File blimp/net/helium/bias.h (right): https://codereview.chromium.org/2400303002/diff/60001/blimp/net/helium/bias.h... blimp/net/helium/bias.h:5: #ifndef BLIMP_NET_HELIUM_BIAS_H On 2016/10/13 22:03:36, Kevin M wrote: > We'll need a home for RunningAs, too. How about syncable_common.h? sgtm https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:98: last_modified_ = last_modified_.MergeWith(to); do you need to merge in the case where you are not updating the data? https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:128: EXPECT_EQ(123, client->value()); I would always assert the state in both sides. Then this way you could parameterize the similar tests into helper functions. For example: ApplyChangesetConflictClientWinsClientDoesNotChangeState and ApplyChangesetConflictEngineWinsClientChangesState https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:69: TEST_F(SyncablePrimitiveSerializerTest, SerializesEmptyString) { Create helper methods for testing ints/strings?
https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:32: const RegisterType& value() const; Hmm. This should probably use a similar naming convention as Set. Since Set can't be made hacker_case because its impl is >1 line, should we just make the getter into "Get()"? Get() and Set() seem like good companion names. Apologies for the churn. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:58: : version_gen_(version_gen), last_modified_(version_gen->current()) { DCHECK(version_gen_) https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:58: : version_gen_(version_gen), last_modified_(version_gen->current()) { value_() so that it gets properly initialized to the type-specific default? Or should we add a check to ensure that the value can't be accessed without first being set via Set() or ApplyChangeset()? https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:59: locally_owned_ = nit: this could be turned into an initialization list member too, but I'm fine with this too https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_common.h (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_common.h:10: enum class Bias { ClientWins, EngineWins }; Recommend renaming these to Client, Engine. If this is used for OwnedRegister, there is no "winner" since there should be no conflicts. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:8: stdint.h? https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:17: } Add newline between methods and use comments to group methods instead, e.g. // ----------------------------- // int32_t serialization methods bool SyncablePr... https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:21: return input_stream->ReadVarint32((uint32_t*)value); Never use C-style casting. static_cast<> would work better here. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:34: if (input_stream->ReadVarint32(&length)) { Should we have a sanity check of some sort here? It's obviously wrong if we get a length that exceeds the maximum packet length, for example. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:37: SerializeBetween<int32_t>(input, &output); These tests should exercise serializing multiple values in a batch, and then deserializing those values. That's a better approximation of how we these streams will be transmitted as consecutive values over streams. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:66: EXPECT_EQ("Hello, World", output); Why aren't we checking for round tripping inside the helper method instead? Check this out, it seems like a good structure for these sorts of tests: https://cs.chromium.org/chromium/src/blimp/net/helium/version_vector_unittest...
PTAL https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:32: const RegisterType& value() const; On 2016/10/17 22:19:45, Kevin M wrote: > Hmm. This should probably use a similar naming convention as Set. Since Set > can't be made hacker_case because its impl is >1 line, should we just make the > getter into "Get()"? Get() and Set() seem like good companion names. > > Apologies for the churn. Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:58: : version_gen_(version_gen), last_modified_(version_gen->current()) { On 2016/10/17 22:19:45, Kevin M wrote: > DCHECK(version_gen_) Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:59: locally_owned_ = On 2016/10/17 22:19:45, Kevin M wrote: > nit: this could be turned into an initialization list member too, but I'm fine > with this too I thought it might be a little complex for an initialization list member. Is there some style-guide rule regarding this? https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register.h:98: last_modified_ = last_modified_.MergeWith(to); On 2016/10/17 22:09:51, scf wrote: > do you need to merge in the case where you are not updating the data? Well, if there's a conflict but we win, in my mind we should still update our |last_modified_| to show that we're at x revision for remote. In reality it won't affect functionality, but I see no harm in keeping it this way. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... File blimp/net/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/lww_re... blimp/net/helium/lww_register_unittest.cc:128: EXPECT_EQ(123, client->value()); On 2016/10/17 22:09:51, scf wrote: > I would always assert the state in both sides. Then this way you could > parameterize the similar tests into helper functions. For example: > ApplyChangesetConflictClientWinsClientDoesNotChangeState and > ApplyChangesetConflictEngineWinsClientChangesState Well, at least the way it's currently working, I only sync in one direction, so checking the unsynced one wouldn't make too much sense, and then it's not clear if I would sync one side and then the other, or sync both directions with changesets created before the sync. I think the way it's currently done is pretty clear and concise, but if you still have doubts we should discuss. Thanks https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_common.h (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_common.h:10: enum class Bias { ClientWins, EngineWins }; On 2016/10/17 22:19:45, Kevin M wrote: > Recommend renaming these to Client, Engine. If this is used for OwnedRegister, > there is no "winner" since there should be no conflicts. Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:8: On 2016/10/17 22:19:45, Kevin M wrote: > stdint.h? Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:17: } On 2016/10/17 22:19:45, Kevin M wrote: > Add newline between methods and use comments to group methods instead, > > e.g. > // ----------------------------- > // int32_t serialization methods > > bool SyncablePr... Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:21: return input_stream->ReadVarint32((uint32_t*)value); On 2016/10/17 22:19:45, Kevin M wrote: > Never use C-style casting. static_cast<> would work better here. Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer.cc:34: if (input_stream->ReadVarint32(&length)) { On 2016/10/17 22:19:45, Kevin M wrote: > Should we have a sanity check of some sort here? It's obviously wrong if we get > a length that exceeds the maximum packet length, for example. Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... File blimp/net/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:37: SerializeBetween<int32_t>(input, &output); On 2016/10/17 22:19:46, Kevin M wrote: > These tests should exercise serializing multiple values in a batch, and then > deserializing those values. That's a better approximation of how we these > streams will be transmitted as consecutive values over streams. Done. https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:66: EXPECT_EQ("Hello, World", output); On 2016/10/17 22:19:46, Kevin M wrote: > Why aren't we checking for round tripping inside the helper method instead? > > Check this out, it seems like a good structure for these sorts of tests: > > https://cs.chromium.org/chromium/src/blimp/net/helium/version_vector_unittest... Discussed offline. Refactored current templated-helper-method approach instead of moving to TEST_P since TEST_P doesn't deal well with different types https://codereview.chromium.org/2400303002/diff/80001/blimp/net/helium/syncab... blimp/net/helium/syncable_primitive_serializer_unittest.cc:69: TEST_F(SyncablePrimitiveSerializerTest, SerializesEmptyString) { On 2016/10/17 22:09:51, scf wrote: > Create helper methods for testing ints/strings? Done.
https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register.h:51: bool value_set_; Suggestion: you can use an inline initializer here https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:20: VersionVectorGenerator client_gen_; Fields must come after methods. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:28: void set_bias(Bias bias) { bias_ = bias; } Do we need setters? Can't the test code just access bias_ directly? https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:30: LwwRegister<int>* getClient() { Camel casing isn't allowed as per the C++ style guide. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:43: engine_lww_register_ = base::MakeUnique<LwwRegister<int>>( Why do we need to lazily instantiate this? Why can't we just create them all the time in SetUp()? https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:58: void Sync(LwwRegister<int>* from_lww_register, Comment on this method https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:144: TEST_F(LwwRegisterTest, ApplyChangesetConflictEngineWinsClientChangesState) { Such a literal description doesn't lend itself well to brevity. Can you summarize this more succinctly? ApplyConflictEngineWins? https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:157: ApplyChangesetConflictEngineWinsRemoteDoesNotChangeState) { You can probably merge the postcondition checks in these "Conflict + Engine Wins" test cases since the test setup is identical. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:26: // serialization and one for deserialization that can be called for any "Any" doesn't seem accurate here, as we only support the types that are explicitly called out here.
PTAL https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register.h:51: bool value_set_; On 2016/10/19 00:27:08, Kevin M wrote: > Suggestion: you can use an inline initializer here Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:20: VersionVectorGenerator client_gen_; On 2016/10/19 00:27:08, Kevin M wrote: > Fields must come after methods. Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:28: void set_bias(Bias bias) { bias_ = bias; } On 2016/10/19 00:27:08, Kevin M wrote: > Do we need setters? Can't the test code just access bias_ directly? Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:30: LwwRegister<int>* getClient() { On 2016/10/19 00:27:08, Kevin M wrote: > Camel casing isn't allowed as per the C++ style guide. Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:43: engine_lww_register_ = base::MakeUnique<LwwRegister<int>>( On 2016/10/19 00:27:08, Kevin M wrote: > Why do we need to lazily instantiate this? Why can't we just create them all the > time in SetUp()? Discussed offline and using Initialize method instead https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:58: void Sync(LwwRegister<int>* from_lww_register, On 2016/10/19 00:27:08, Kevin M wrote: > Comment on this method Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:144: TEST_F(LwwRegisterTest, ApplyChangesetConflictEngineWinsClientChangesState) { On 2016/10/19 00:27:08, Kevin M wrote: > Such a literal description doesn't lend itself well to brevity. Can you > summarize this more succinctly? > > ApplyConflictEngineWins? Done. https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:157: ApplyChangesetConflictEngineWinsRemoteDoesNotChangeState) { On 2016/10/19 00:27:08, Kevin M wrote: > You can probably merge the postcondition checks in these "Conflict + Engine > Wins" test cases since the test setup is identical. In one case, I sync to the engine, in the other I sync to the client. I can't just call one sync and then the other, since then the second sync is different. So in order to properly emulate it, I'd have to get the changesets from each one and then apply each one (splitting the Sync method in half essentially). I think this is more complexity than it's worth, but we should discuss offline https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/100001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:26: // serialization and one for deserialization that can be called for any On 2016/10/19 00:27:08, Kevin M wrote: > "Any" doesn't seem accurate here, as we only support the types that are > explicitly called out here. Done.
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register.h:26: LwwRegister(VersionVectorGenerator* version_gen, Peer bias, Peer running_as); Since I end up just storing whether bias == running_as, would it make sense to just take a bool?
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:62: from_lww_register->CreateChangesetToCurrent(from.Invert(), &output_stream); do you need to invert here? i thought it was just remote https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:16: constexpr uint32_t kMaxStringLength = (10 << 20); // 10MB move this to common? i will probably need something similar in USet https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:48: if (input_stream->ReadVarint32(&length) && length < kMaxStringLength) { ideally the kMaxStringLength case would be detected at the serializer side. and probably failing serialization. here in the deserialization you already sent the data across the wire and are leaving the stream cursor in a invalid state. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:28: class BLIMP_HELIUM_EXPORT SyncablePrimitiveSerializer { does it need to export here? https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer_unittest.cc:57: output_stream.WriteString("This string is 20MB, I swear!"); lol
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:62: from_lww_register->CreateChangesetToCurrent(from.Invert(), &output_stream); On 2016/10/19 17:23:03, scf wrote: > do you need to invert here? i thought it was just remote Yep. |from| comes from |to_lww_register|, and so it needs to be inverted when handed to |from_lww_register|. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:48: if (input_stream->ReadVarint32(&length) && length < kMaxStringLength) { On 2016/10/19 17:23:03, scf wrote: > ideally the kMaxStringLength case would be detected at the serializer side. and > probably failing serialization. here in the deserialization you already sent the > data across the wire and are leaving the stream cursor in a invalid state. I agree, although in my mind this is more of a sanity check that the data isn't corrupt. One doubt I have about failing on Serialization is that AFAIK we were assuming serialization couldn't fail (I could be wrong on this). I'm sure Kevin has some thoughts on this. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:28: class BLIMP_HELIUM_EXPORT SyncablePrimitiveSerializer { On 2016/10/19 17:23:03, scf wrote: > does it need to export here? Yes, or else the unittests don't link properly
Very close, would just like to see the serialization tests get beefed up a bit. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register.h:26: LwwRegister(VersionVectorGenerator* version_gen, Peer bias, Peer running_as); On 2016/10/19 16:42:49, steimel wrote: > Since I end up just storing whether bias == running_as, would it make sense to > just take a bool? That's up to you, but IMO explicitly breaking out the parameters makes things more explicit? https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:46: Peer bias_; Remove this field? https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:50: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:144: Clean tests! https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:48: if (input_stream->ReadVarint32(&length) && length < kMaxStringLength) { On 2016/10/19 17:37:54, steimel wrote: > On 2016/10/19 17:23:03, scf wrote: > > ideally the kMaxStringLength case would be detected at the serializer side. > and > > probably failing serialization. here in the deserialization you already sent > the > > data across the wire and are leaving the stream cursor in a invalid state. > I agree, although in my mind this is more of a sanity check that the data isn't > corrupt. Yup! This is more about validating the payload contents than the payload size itself. > One doubt I have about failing on Serialization You are correct that serialization shouldn't fail, but this code is failing on deserialization. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer_unittest.cc:42: Test mixed-type serialization? e.g. <int><string>, <string><int>
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:48: if (input_stream->ReadVarint32(&length) && length < kMaxStringLength) { On 2016/10/19 17:49:11, Kevin M wrote: > On 2016/10/19 17:37:54, steimel wrote: > > On 2016/10/19 17:23:03, scf wrote: > > > ideally the kMaxStringLength case would be detected at the serializer side. > > and > > > probably failing serialization. here in the deserialization you already sent > > the > > > data across the wire and are leaving the stream cursor in a invalid state. > > I agree, although in my mind this is more of a sanity check that the data > isn't > > corrupt. > > Yup! This is more about validating the payload contents than the payload size > itself. > > > One doubt I have about failing on Serialization > > You are correct that serialization shouldn't fail, but this code is failing on > deserialization. I think the size limit should rarely happen, and in case it happens most likely something is corrupt, which I think we should fail during serialization.
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_common.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_common.h:11: enum class Peer { Client, Engine }; CLIENT, ENGINE
https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register.h:45: VersionVectorGenerator* version_gen_; Does the LWW_Register take ownership over the VersionVectorGenerator? If so, unique_ptr for this.
PTAL https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register.h:26: LwwRegister(VersionVectorGenerator* version_gen, Peer bias, Peer running_as); On 2016/10/19 17:49:11, Kevin M wrote: > On 2016/10/19 16:42:49, steimel wrote: > > Since I end up just storing whether bias == running_as, would it make sense to > > just take a bool? > > That's up to you, but IMO explicitly breaking out the parameters makes things > more explicit? Agreed. I'll leave it https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register.h:45: VersionVectorGenerator* version_gen_; On 2016/10/20 00:20:16, CJ wrote: > Does the LWW_Register take ownership over the VersionVectorGenerator? If so, > unique_ptr for this. I believe a single, global generator will be used with all syncables, so I won't own it https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... File blimp/helium/lww_register_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:46: Peer bias_; On 2016/10/19 17:49:11, Kevin M wrote: > Remove this field? Done. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:50: }; On 2016/10/19 17:49:11, Kevin M wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/lww_regis... blimp/helium/lww_register_unittest.cc:144: On 2016/10/19 17:49:11, Kevin M wrote: > Clean tests! Thanks! https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_common.h (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_common.h:11: enum class Peer { Client, Engine }; On 2016/10/19 21:46:56, Kevin M wrote: > CLIENT, ENGINE Done. https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/120001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer_unittest.cc:42: On 2016/10/19 17:49:11, Kevin M wrote: > Test mixed-type serialization? e.g. <int><string>, <string><int> Done.
lgtm https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:42: DCHECK(value.length() < kMaxStringLength); DCHECK_LT https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:57: // ------ Moar dashes? https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:76: return true; Can you flip the conditional so that "return true" is executed at the bottom? It's more readable to have error handling/returns happen inside the conditionals, and letting the the main execution flow belong to the happy path. https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:30: class BLIMP_HELIUM_EXPORT SyncablePrimitiveSerializer { CodedValueSerializer? https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer_unittest.cc:100: output_stream.WriteVarint32(20 << 20); // 20MB "20MB, equal to kMaxStringLength"
PTAL https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:42: DCHECK(value.length() < kMaxStringLength); On 2016/10/20 00:38:24, Kevin M wrote: > DCHECK_LT Done. https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:57: // ------ On 2016/10/20 00:38:24, Kevin M wrote: > Moar dashes? Done. https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.cc:76: return true; On 2016/10/20 00:38:24, Kevin M wrote: > Can you flip the conditional so that "return true" is executed at the bottom? > It's more readable to have error handling/returns happen inside the > conditionals, and letting the the main execution flow belong to the happy path. Done. https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer.h (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer.h:30: class BLIMP_HELIUM_EXPORT SyncablePrimitiveSerializer { On 2016/10/20 00:38:25, Kevin M wrote: > CodedValueSerializer? Done. https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... File blimp/helium/syncable_primitive_serializer_unittest.cc (right): https://codereview.chromium.org/2400303002/diff/140001/blimp/helium/syncable_... blimp/helium/syncable_primitive_serializer_unittest.cc:100: output_stream.WriteVarint32(20 << 20); // 20MB On 2016/10/20 00:38:25, Kevin M wrote: > "20MB, equal to kMaxStringLength" Done, but with "greater than", since kMax is 10MB
lgtm https://codereview.chromium.org/2400303002/diff/160001/blimp/helium/coded_val... File blimp/helium/coded_value_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/160001/blimp/helium/coded_val... blimp/helium/coded_value_serializer.cc:18: constexpr uint32_t kMaxStringLength = (10 << 20); // 10MB Add a comment saying this constant is arbitrary and is just a sanity check.
https://codereview.chromium.org/2400303002/diff/160001/blimp/helium/coded_val... File blimp/helium/coded_value_serializer.cc (right): https://codereview.chromium.org/2400303002/diff/160001/blimp/helium/coded_val... blimp/helium/coded_value_serializer.cc:18: constexpr uint32_t kMaxStringLength = (10 << 20); // 10MB On 2016/10/20 17:11:22, Kevin M wrote: > Add a comment saying this constant is arbitrary and is just a sanity check. Done.
The CQ bit was checked by steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scf@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2400303002/#ps180001 (title: "Add comment on max string length constant")
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.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add LwwRegister CRDT BUG=650466 ========== to ========== Add LwwRegister CRDT BUG=650466 Committed: https://crrev.com/cc4584d7c74a937b617180bc69a5816b49f004b3 Cr-Commit-Position: refs/heads/master@{#426549} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cc4584d7c74a937b617180bc69a5816b49f004b3 Cr-Commit-Position: refs/heads/master@{#426549} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
