CRDT.ORMap serialization #507 #598

yim-lee · 2020-04-29T05:27:30Z

Motivation:
CRDT.ORMap does not conform to ProtobufRepresentable

Modiifications:

Modified ORMap so it doesn't require a closure in initializer
Conform ORMap and ORMapDelta to ProtobufRepresentable

Result:
Part of #507

Motivation: `CRDT.ORMap` does not conform to `ProtobufRepresentable` Modiifications: - Modified `ORMap` so it doesn't require a closure in initializer - Conform `ORMap` and `ORMapDelta` to `ProtobufRepresentable` Result: Part of #507

Protos/CRDT/CRDT.proto

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

Protos/CRDT/CRDT.proto

ktoso · 2020-04-29T05:39:35Z

Protos/CRDT/CRDT.proto

+
+message CRDTORMap {
+    message Delta {
+        CRDTORMapValue defaultValue = 1;


carrying the default a bit weird I guess... it's because we want to merge(default, this value) if the other node does not have a value? Would the other node's defaultValue not be usable instead?

It's for merge, update, and reset--more so for update which takes a mutator closure and expects Value as param, so we make a copy of defaultValue in case there is no existing value.

Perhaps we can modify the mutator to accept Value? and return Value, so the caller is responsible for providing the default if needed, then we can get rid of this defaultValue I think...

modifications would look like thing(.empty) { $0.add("hello") I guess then right? That's bearable (AFAIR also how Akka's Map worked)

No, not so easy. We need something for merge in case key doesn't have value locally, and we can't just copy other node's value because the replicaID is expected to be different.

I see what you meant by your first point now. We don't have to transport defaultValue--we would still require it in the initializer, but we allow it to be nil if we got it from remote. All we care about is local ORMap has a defaultValue. The remote one doesn't matter.

Right yes; I'm not sure how it complicates end user API though 🤔 Curious what you can figure out

The only change for users is they pass in an object instead of closure. Looks nicer actually.

https://github.com/apple/swift-distributed-actors/pull/598/files#diff-a7c729fc83691600e9c28aa7244e0986R341

https://github.com/apple/swift-distributed-actors/pull/598/files#diff-a34790d01eb2ce2b84d17d9180b63d7bR27

It does look nicer! :)

Protos/CRDT/CRDT.proto

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

Sources/DistributedActors/CRDT/Types/CRDT+StateBased.swift

Sources/DistributedActors/CRDT/Types/CRDT+LWWRegister.swift

Sources/DistributedActors/CRDT/Types/CRDT+StateBased.swift

yim-lee · 2020-04-29T17:58:21Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

+        /// but it **is** required in the initializer to ensure that **local** `ORMap` has `defaultValue`
+        /// for `merge`, `update`, etc. In those methods `defaultValue` is required in case **local**
+        /// `ORMap` does not have an existing value for the given `key`.
+        let defaultValue: Value?


This is now optional when constructed from protobuf

yim-lee · 2020-04-29T17:59:03Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

        }

-        init(replicaID: ReplicaID, valueInitializer: @escaping () -> Value) {
+        init(replicaID: ReplicaID, defaultValue: Value) {


defaultValue is required for local construction

yim-lee · 2020-04-29T17:59:53Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

        }

        public mutating func update(key: Key, mutator: (inout Value) -> Void) {
+            guard let defaultValue = self.defaultValue else {


Added these guards as a result of making defaultValue optional

yim-lee · 2020-04-29T18:00:03Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift


            // Apply `mutator` to the value then save it to state. Create `Value` if needed.
-            var value = self._values[key] ?? self.valueInitializer()
+            var value = self._values[key] ?? defaultValue


Assuming value semantics

yim-lee · 2020-04-29T18:06:09Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

+        let defaultValue: Value?

-        init(keys: ORSet<Key>.Delta, values: [Key: Value], valueInitializer: @escaping () -> Value) {
+        init(keys: ORSet<Key>.Delta, values: [Key: Value], defaultValue: Value?) {


Delta may be constructed from deserialized ORMap (https://github.com/apple/swift-distributed-actors/pull/598/files#diff-6d3797019f1301f2492ee9aed777e118R77), which has defaultValue as nil, so here we have to allow Value? not Value.

yim-lee · 2020-04-29T18:06:40Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

+/// Additional `ORMap` methods when `Value` type conforms to `ResettableCRDT`.
 public protocol ORMapWithResettableValue: ORMapWithUnsafeRemove {
-    /// Resets value for `key` to `defaultValue` provided in `init`.
+    /// Resets value for `key` by calling `ResettableCRDT.reset()`.


Corrected the comments

yim-lee · 2020-04-29T18:06:50Z

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

            // If `k` is not found in `self` then create a new `Value` instance.
            // We must NOT copy `other`'s value directly to `self` because the two should have different replica IDs.
-            var lv: Value = self[k] ?? valueInitializer()
+            var lv: Value = self[k] ?? defaultValue


Assuming value semantics

yim-lee · 2020-04-29T18:07:42Z

Tests/DistributedActorsTests/CRDT/Protobuf/CRDT+SerializationTests.swift

+            let serialized = try system.serialization.serialize(map)
+            let deserialized = try system.serialization.deserialize(as: CRDT.ORMap<String, CRDT.ORSet<String>>.self, from: serialized)
+
+            "\(deserialized.replicaID)".shouldContain("actor:sact://CRDTSerializationTests@localhost:9001/user/alpha")


Sorry, this is another one of those localhost to 127.0.0.1 change

Ok no problem, sorry about the change 🙇 In the end it's more consistent though I think..

In the end it's more consistent though

+1. 🙏

yim-lee · 2020-04-29T18:17:43Z

Test failure was #463

ktoso · 2020-04-30T02:20:37Z

Sources/DistributedActors/CRDT/Types/CRDT+LWWMap.swift

    /// - SeeAlso: `CRDT.ORMap`
    /// - SeeAlso: `CRDT.LWWRegister`
-    public struct LWWMap<Key: Codable & Hashable, Value: ActorMessage>: NamedDeltaCRDT, LWWMapOperations {
+    public struct LWWMap<Key: Codable & Hashable, Value: Codable>: NamedDeltaCRDT, LWWMapOperations {


Good 👍

ActorMessage really only a marker for us to grep over them when we : ActorMessage things, Codable in most places is the right thing 👍

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift

ktoso

I'm a bit iffy if the optional is good enough as I've not dug very deep here but if there's issues we'll hit them during replication work so let's solve then 👍

LGTM

yim-lee · 2020-04-30T03:37:25Z

I'm a bit iffy if the optional is good enough

yeah same. worst case we just add it back to the payload...

CRDT.ORMap serialization #507

cc9117a

Motivation: `CRDT.ORMap` does not conform to `ProtobufRepresentable` Modiifications: - Modified `ORMap` so it doesn't require a closure in initializer - Conform `ORMap` and `ORMapDelta` to `ProtobufRepresentable` Result: Part of #507

yim-lee commented Apr 29, 2020

View reviewed changes

Protos/CRDT/CRDT.proto Show resolved Hide resolved

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift Show resolved Hide resolved

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift Outdated Show resolved Hide resolved

fix formatting

bac634c

ktoso reviewed Apr 29, 2020

View reviewed changes

Remove CloneableCRDT. Assume CRDTs have value semantics.

908e7b9

ktoso reviewed Apr 29, 2020

View reviewed changes

Sources/DistributedActors/CRDT/Types/CRDT+LWWRegister.swift Outdated Show resolved Hide resolved

Sources/DistributedActors/CRDT/Types/CRDT+StateBased.swift Show resolved Hide resolved

Sources/DistributedActors/CRDT/Types/CRDT+StateBased.swift Outdated Show resolved Hide resolved

remove defaultValue from proto messages

8d2ac88

yim-lee commented Apr 29, 2020

View reviewed changes

yim-lee requested a review from ktoso April 29, 2020 18:19

ktoso reviewed Apr 30, 2020

View reviewed changes

Sources/DistributedActors/CRDT/Types/CRDT+ORMap.swift Outdated Show resolved Hide resolved

ktoso added this to the 0.5.0 - Cluster Hardening milestone Apr 30, 2020

Apply suggestions from code review

d74ecdd

ktoso approved these changes Apr 30, 2020

View reviewed changes

ktoso merged commit b423553 into apple:master Apr 30, 2020

ktoso deleted the ormap-seri branch April 30, 2020 02:52

ktoso added the t:crdt label Apr 30, 2020

CRDT.ORMap serialization #507 #598

CRDT.ORMap serialization #507 #598

Uh oh!

Conversation

yim-lee commented Apr 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee commented Apr 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

yim-lee commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yim-lee Apr 29, 2020 •

edited

Loading