Add LwwRegister CRDT

blimp/net/helium/lww_register.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BLIMP_NET_HELIUM_LWW_REGISTER_H_
+#define BLIMP_NET_HELIUM_LWW_REGISTER_H_
+
+#include "base/memory/ptr_util.h"
+#include "blimp/common/proto/helium.pb.h"
+#include "blimp/net/blimp_net_export.h"
+#include "blimp/net/helium/lww_register_serializer.h"
+#include "blimp/net/helium/syncable.h"
+#include "blimp/net/helium/vector_clock.h"
+#include "blimp/net/helium/vector_clock_generator.h"
+
+namespace blimp {
+
+enum class RunningAs { RunningAsClient, RunningAsEngine };

[scf, 2016/10/10 19:45:46]

This should probably go into its own file as its going to be reused elsewhere

[CJ, 2016/10/10 20:25:55]

Could simplify the enums as just "Client" or "Engine", since it would come up as
RunningAs::Client etc. 

[Kevin M, 2016/10/10 20:50:21]

+1. "enum class" is nice for enforcing prefixes.

[steimel, 2016/10/11 16:39:17]

Done.

+enum class LwwBias { ClientWins, EngineWins };

[scf, 2016/10/10 19:45:47]

move this inside LwwRegister

[Kevin M, 2016/10/10 20:50:21]

Could this just be factored out as "Bias"?

[steimel, 2016/10/11 16:39:17]

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)

+
+template <class RegisterType, LwwBias bias>

[Kevin M, 2016/10/10 20:50:21]

Add comments

[steimel, 2016/10/11 16:39:17]

Done.

[steimel, 2016/10/11 16:39:17]

Done.

+class BLIMP_NET_EXPORT LwwRegister
+    : Syncable<proto::LwwRegisterChangesetMessage> {
+ public:
+  explicit LwwRegister(VectorClockGenerator* clock_gen, RunningAs running_as);

[CJ, 2016/10/07 22:07:54]

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, 2016/10/07 22:53:39]

You actually can implicitly construct with multiple parameters
(http://en.cppreference.com/w/cpp/language/explicit), though it's not super
common afaik

[Kevin M, 2016/10/10 20:50:21]

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.

[steimel, 2016/10/11 16:39:17]

Done.

+  ~LwwRegister();
+
+  void set(const RegisterType& state);

[scf, 2016/10/10 19:45:47]

rename to |Set| as its a "complex" operation

[steimel, 2016/10/11 16:39:17]

Done.

+
+  const RegisterType& state() const;

[scf, 2016/10/10 19:45:46]

rename to get()

[steimel, 2016/10/11 16:39:17]

Done.

+
+  // Syncable implementation.
+  bool ModifiedSince(const VectorClock& from) const override;
+  std::unique_ptr<proto::LwwRegisterChangesetMessage> CreateChangesetToCurrent(
+      const VectorClock& from) override;
+  void ApplyChangeset(
+      const VectorClock& from,
+      const VectorClock& to,
+      std::unique_ptr<proto::LwwRegisterChangesetMessage> changeset) override;
+  void ReleaseCheckpointsBefore(const VectorClock& checkpoint) override;
+
+ private:
+  VectorClockGenerator* clock_gen_;
+  VectorClock last_modified_;
+  RunningAs running_as_;
+  RegisterType state_;

[scf, 2016/10/10 19:45:47]

provably rename to value_

[steimel, 2016/10/11 16:39:17]

Done.

+
+  bool LocalWinsConflict() const;
+
+  DISALLOW_COPY_AND_ASSIGN(LwwRegister);
+};
+
+template <class RegisterType, LwwBias bias>
+LwwRegister<RegisterType, bias>::LwwRegister(VectorClockGenerator* clock_gen,
+                                             RunningAs running_as)
+    : clock_gen_(clock_gen),
+      last_modified_(clock_gen->current()),
+      running_as_(running_as) {}
+
+template <class RegisterType, LwwBias bias>
+LwwRegister<RegisterType, bias>::~LwwRegister() = default;

[scf, 2016/10/10 19:45:46]

dumb question, does the compiler complains this is complex? Otherwise could move
this to the declaration

[steimel, 2016/10/11 16:39:17]

Done.

+
+template <class RegisterType, LwwBias bias>
+void LwwRegister<RegisterType, bias>::set(const RegisterType& state) {
+  state_ = state;
+  clock_gen_->Increment();
+  last_modified_ = clock_gen_->current();
+}
+
+template <class RegisterType, LwwBias bias>
+const RegisterType& LwwRegister<RegisterType, bias>::state() const {
+  return state_;
+}
+
+template <class RegisterType, LwwBias bias>
+bool LwwRegister<RegisterType, bias>::ModifiedSince(
+    const VectorClock& from) const {
+  return from.local_revision() < last_modified_.local_revision();
+}
+
+template <class RegisterType, LwwBias bias>
+std::unique_ptr<proto::LwwRegisterChangesetMessage>
+LwwRegister<RegisterType, bias>::CreateChangesetToCurrent(
+    const VectorClock& from) {
+  std::unique_ptr<proto::LwwRegisterChangesetMessage> changeset =
+      base::MakeUnique<proto::LwwRegisterChangesetMessage>();
+  LwwRegisterSerializer<RegisterType>::SerializeLwwRegister(state_,
+                                                            changeset.get());
+  return changeset;
+}
+
+template <class RegisterType, LwwBias bias>
+void LwwRegister<RegisterType, bias>::ApplyChangeset(
+    const VectorClock& from,
+    const VectorClock& to,
+    std::unique_ptr<proto::LwwRegisterChangesetMessage> changeset) {
+  switch (last_modified_.CompareTo(to)) {

[scf, 2016/10/10 19:45:47]

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.

[steimel, 2016/10/11 16:39:17]

Done.

+    case VectorClock::Comparison::GreaterThan:
+      // We're ahead of this changeset, so do nothing.
+      break;
+    case VectorClock::Comparison::LessThan:
+      // We're behind this changeset, so set to given value.
+      LwwRegisterSerializer<RegisterType>::DeserializeLwwRegister(
+          state_, changeset.get());
+      break;
+    case VectorClock::Comparison::EqualTo:
+      // We're at the same revision as this changeset, so do nothing.
+      break;
+    case VectorClock::Comparison::Conflict:
+      // Conflict, so set to given value if we lose conflicts.
+      if (!LocalWinsConflict()) {
+        LwwRegisterSerializer<RegisterType>::DeserializeLwwRegister(
+            state_, changeset.get());
+      }
+      break;
+  }
+
+  last_modified_ = last_modified_.MergeWith(to);
+}
+
+template <class RegisterType, LwwBias bias>
+void LwwRegister<RegisterType, bias>::ReleaseCheckpointsBefore(
+    const VectorClock& checkpoint) {
+  // no-op
+}
+
+template <class RegisterType, LwwBias bias>

[Kevin M, 2016/10/10 20:50:21]

Move this to wherever it is that we will be stashing Bias and OwnedBy?

[steimel, 2016/10/11 16:39:17]

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?

+bool LwwRegister<RegisterType, bias>::LocalWinsConflict() const {
+  switch (running_as_) {
+    case RunningAs::RunningAsClient:
+      return bias == LwwBias::ClientWins;
+    case RunningAs::RunningAsEngine:
+      return bias == LwwBias::EngineWins;
+  }
+}
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_HELIUM_LWW_REGISTER_H_