CRDT Serialization #92

ktoso · 2019-09-04T05:06:25Z

Serializers and infra for CRDT Serialization

If merging, please rebase merge, co creds remain for Yim's initial work

Motivation:

Unlocks direct (and gossip) replication CRDT: "Direct Replication" / at-Consistency writes #27

Modifications:

Continues the work from [WIP] CRDT Serialization #81 while @yim-lee's on time off
protobuf serialization for ORSet and GCounter (and all things they need)
some tests of those serializations
adds "registerBoxing" though I hope to throw it out the window soon -- it is too complex to expect normal users to do this when they register a type for serialization; See Workaround / remove Serialization.registerBoxing #91

Result:

serializable .write message as well as core CRDT types including GCounter and ORSet

ktoso · 2019-09-04T05:06:46Z

I'll rebase/squash now, though now lunch :)

Protos/CRDT/CRDTReplication.proto

drexin

Publishing the partial review and continue later.

Sources/DistributedActors/CRDT/CRDT+Replication.swift

Sources/DistributedActors/CRDT/Protobuf/CRDT+Serialization.swift

drexin · 2019-09-04T20:21:57Z

Sources/DistributedActors/CRDT/Protobuf/CRDT+Serialization.swift

+        self.vv = try .init(fromProto: proto.versionVector, context: context)
+        self.gaps = []
+        self.gaps.reserveCapacity(proto.gaps.count)
+        for versionDot in proto.gaps {


Use map instead?

Hm can't on this one since typemismatch, or I'd have to first make the thing to a Set<> not sure if that's "free"?

Sources/DistributedActors/CRDT/Protobuf/CRDT+Serialization.swift

Sources/DistributedActors/CRDT/Protobuf/CRDTEnvelope+Serialization.swift

drexin · 2019-09-05T01:32:20Z

Sources/DistributedActors/Serialization.swift

+    /// MUST NOT be invoked after initialization of serialization. // TODO: enforce this perhaps? Or we'd need concurrent maps otherwise... should be just a set-once thing tbh
+    // TODO: Not sure if there is a way around this, or something similar, as we always need to make the id -> type jump eventually.
+    // FIXME: We should not need this; It's too complex for end users (having to call "random" boxings when they register a type does not sound sane
+    public mutating func registerBoxing<M, Box>(from messageType: M.Type, into boxType: Box.Type, _ boxer: @escaping (M) -> Box) {


I need to spend some time understanding this. Will continue here later.

Caught up on chat, seems decision is to revisit as swift painpoint and seek help during review. For now it unlocks us at least.

We could also change our messages to avoid erasure, though question is if that's the answer for our users or not.

FYI @yim-lee

ok 👍

revisit as swift painpoint

What would the pain-point be?

We could also change our messages to avoid erasure

If we could use CvRDT as type rather than just generic constraint. Would that be considered a pain-point? :)

What would the pain-point be?

I made it into a ticket #91
I think what would work is if we were able to ask "for any ORSet" but we can't do this since ObjectIdentifier of a type is always the ORSet<OfSomething>; I think if we could check this then we could automatically run the "box it using the thing" code... 🤔 Have not made sure yet...

We could also change our messages to avoid erasure

If we could use CvRDT as type rather than just generic constraint. Would that be considered a pain-point? :)

So... then we would not have to do any of the boxing at all right? I guess that would help then 🤔

Can we add an untyped interface in between and register that for serialization? That could then have a property that's the MetaType of the contained type. I.e.

protocol _ORSet { var elementMetaType: AnyMetaType }

Sources/DistributedActors/Serialization.swift

ktoso · 2019-09-05T06:20:01Z

Addressed review; Watch out when merging though; this is best merged as a rebase or manually squashing things when doing so, so we keep @yim-lee's commit separate.

ktoso · 2019-09-05T10:13:08Z

Failure was #17, tied to investigate

ktoso · 2019-09-05T10:13:15Z

@swift-server-bot test this please

drexin · 2019-09-05T15:41:13Z

#98

03:24:59 Test Case 'VersionVectorTests.test_Dot_sort_shouldBeByReplicaThenByVersion' started at 2019-09-05 10:24:59.586
03:24:59 /code/Tests/DistributedActorsTests/Clocks/VersionVectorTests.swift:194: error: VersionVectorTests.test_Dot_sort_shouldBeByReplicaThenByVersion : XCTAssertEqual failed: ("[dot:(actor:/user/B,1), dot:(actor:/user/B,2), dot:(actor:/user/A,3), dot:(actor:/user/C,5)]") is not equal to ("[dot:(actor:/user/A,3), dot:(actor:/user/B,1), dot:(actor:/user/B,2), dot:(actor:/user/C,5)]") - 
03:24:59         sortedDots.shouldEqual([dot2, dot3, dot1, dot4])
03:24:59                    ^~~~~~~~~~~~
03:24:59 error: [[dot:(actor:/user/B,1), dot:(actor:/user/B,2), dot:(actor:/user/A,3), dot:(actor:/user/C,5)]] does not equal expected: [[dot:(actor:/user/A,3), dot:(actor:/user/B,1), dot:(actor:/user/B,2), dot:(actor:/user/C,5)]]
03:24:59 
03:24:59 Test Case 'VersionVectorTests.test_Dot_sort_shouldBeByReplicaThenByVersion' failed (0.004 seconds)
03:24:59 Test Suite 'VersionVectorTests' failed at 2019-09-05 10:24:59.589
03:24:59 	 Executed 8 tests, with 1 failure (0 unexpected) in 0.194 (0.194) seconds
03:24:59 Test Suite 'WorkerPoolTests' started at 2019-09-05 10:24:59.589

yim-lee · 2019-09-05T17:30:35Z

Watch out when merging though; this is best merged as a rebase or manually squashing things when doing so, so we keep @yim-lee's commit separate.

Don't worry. I won't be offended if my commit were squashed.

yim-lee · 2019-09-05T17:31:24Z

~~Looking through this. Please hold off merging.~~

Done. Thanks.

drexin · 2019-09-05T17:59:14Z

@yim-lee Credit where credit is due :).

drexin · 2019-09-05T18:22:01Z

@swift-server-bot test this please

yim-lee

Thanks so much @ktoso. I think this approach is much cleaner. There might be some confusion around AnyCvRDT and AnyDeltaCRDT (they both represent full CRDT), but not a deal-breaker.

Protos/CRDT/CRDT.proto

Sources/DistributedActors/Clocks/VersionVector.swift

Sources/DistributedActors/Serialization+SerializerId.swift

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

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

ktoso · 2019-09-06T00:47:28Z

Thanks for review! Looking at it :)

Motivation: For direct replication (#27) we need to send CRDTs to remote replicators, therefore they need to be serializable. Modifications: - Define protobufs for CRDTs and conform to `InternalProtobufRepresentable`. - Define protobufs for `CRDT.Replicator.RemoteCommand`. - Special handling in `Serialization` for CRDT serializers. Result: Be able to send `CRDT.Replicator.RemoteCommand` message to remote replicators.

access serializer internally without exposing collection

make boxing a first class citizen, and use for AnyCRDTs formatting +crdt,ser carry delta in serialized gcounters

Allow for ORSet<Int> and test Replicator write containing ORSet

ktoso · 2019-09-06T08:08:58Z

The failure will be fixed in #103

Thanks for review, let's keep moving with the CRDTs :)

I'm a bit confused about #92 (comment)

perhaps let's clean anything I've messed up on those in a follow up PR next week? Hope that's okey.

yim-lee · 2019-09-06T16:54:11Z

perhaps let's clean anything I've messed up on those in a follow up PR

Yep, thanks @ktoso for your help. I will pick up from this next week.

ktoso requested review from yim-lee and drexin September 4, 2019 10:48

ktoso commented Sep 4, 2019

View reviewed changes

Protos/CRDT/CRDTReplication.proto Show resolved Hide resolved

drexin reviewed Sep 5, 2019

View reviewed changes

Sources/DistributedActors/Serialization.swift Outdated Show resolved Hide resolved

ktoso requested a review from drexin September 5, 2019 06:20

yim-lee reviewed Sep 6, 2019

View reviewed changes

ktoso mentioned this pull request Sep 6, 2019

Workaround / remove Serialization.registerBoxing #91

Closed

yim-lee and others added 6 commits September 6, 2019 15:39

Disable overly aggressive swiftformat in specific code spots

3ef49d0

-ser remove ad-hoc and not used ancient system msg serializer

22d84df

access serializer internally without exposing collection

=ser dump serializer ids in specific order

223ab05

Reworking to simplify CRDT serialization using Envelope

c8bd0e5

make boxing a first class citizen, and use for AnyCRDTs formatting +crdt,ser carry delta in serialized gcounters

=ser,crdt Implements ORSet serialization

f1b2f43

Allow for ORSet<Int> and test Replicator write containing ORSet

ktoso merged commit 1870e04 into apple:master Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRDT Serialization #92

CRDT Serialization #92

ktoso commented Sep 4, 2019 •

edited

Loading

ktoso commented Sep 4, 2019

drexin left a comment

drexin Sep 4, 2019

ktoso Sep 5, 2019

drexin Sep 5, 2019

ktoso Sep 5, 2019

yim-lee Sep 5, 2019

ktoso Sep 6, 2019

ktoso Sep 6, 2019

drexin Sep 6, 2019

ktoso commented Sep 5, 2019

ktoso commented Sep 5, 2019

ktoso commented Sep 5, 2019

drexin commented Sep 5, 2019 •

edited

Loading

yim-lee commented Sep 5, 2019

yim-lee commented Sep 5, 2019 •

edited

Loading

drexin commented Sep 5, 2019

drexin commented Sep 5, 2019

yim-lee left a comment

ktoso commented Sep 6, 2019

ktoso commented Sep 6, 2019

yim-lee commented Sep 6, 2019

CRDT Serialization #92

CRDT Serialization #92

Conversation

ktoso commented Sep 4, 2019 • edited Loading

Motivation:

Modifications:

Result:

ktoso commented Sep 4, 2019

drexin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Sep 5, 2019

ktoso commented Sep 5, 2019

ktoso commented Sep 5, 2019

drexin commented Sep 5, 2019 • edited Loading

yim-lee commented Sep 5, 2019

yim-lee commented Sep 5, 2019 • edited Loading

drexin commented Sep 5, 2019

drexin commented Sep 5, 2019

yim-lee left a comment

Choose a reason for hiding this comment

ktoso commented Sep 6, 2019

ktoso commented Sep 6, 2019

yim-lee commented Sep 6, 2019

ktoso commented Sep 4, 2019 •

edited

Loading

drexin commented Sep 5, 2019 •

edited

Loading

yim-lee commented Sep 5, 2019 •

edited

Loading