Implement Datastore class#371
Conversation
Will be better placed in an example
|
One possible change to this PR, that would probably make sense would be to make the |
sccolbert
left a comment
sccolbert
left a comment
There was a problem hiding this comment.
Initial review done. Will do round two after these changes are made.
Thanks!
- Match TS version to that in master - Validate schemas on datastore creation - Have tables collection be a BPlusTree - Throw error / log warning for missing tables. - Implement IMessageHandler instead of `apply()`. - Require broadcastHandler arg on creation. - Store current transaction id on schema.
- Make broadcastHandler optional again, but still readonly. - Add an option transactionIdFactory to override createDuplexId.
|
Thanks for the review. I pushed some fixes to most of the points. Will still need to take them for a test to see that they work as intended, but they are mostly there. The only point I was unsure about is the version number: Why does the version number need to be communicated among the stores? Given the asynchronous nature of the broadcasting, there is likely to be transactions that are created simultaneously from identical versions. |
|
PS: While I did make the broadcast handler a readonly argument, I kept it as optional. This will allow it to be used as a local (undoable) datastore when offline as a fallback. Not committed to that feature though. |
To answer my own question, it is used to determine concurrent changes in peers (same version for two changes implies concurrent changes). |
Copy out the variables from the context in case the context changes between broadcast / change. Also, the broadcast step is put first, as for many broadcast scenarios much of this can happen in parallell.
(no logic change)
|
@sccolbert I've addressed all the points of the current review, so it should be ready for another look. |
Slice vs splice signature difference
| * order to guarantee stability. E.g. a transaction that removes some text | ||
| * must be delivered *after* the transaction that adds that text. Any | ||
| * deviation from this will *not* raise an error, but can lead to | ||
| * diverging state between peers. |
There was a problem hiding this comment.
@sccolbert Regarding this comment: As far as I read the textfield logic (and saw it in action), any attempt to remove text that has not yet been added will be discarded, and the text will remain in the value. Maybe that is because the cemetery attribute of the textfield metadata is not in use? I'll see if I can create an MWE on Monday, so we have something specific to discuss, but I thought I would leave this note for now.
|
Yeah, once you add the cemetery, the causality requirement goes away.
…On Fri, Jun 14, 2019 at 12:12 PM Vidar Tonaas Fauske < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/datastore/src/datastore.ts
<#371 (comment)>:
> @@ -32,15 +48,68 @@ import {
*
* https://en.wikipedia.org/wiki/Conflict-free_replicated_data_type
* https://hal.inria.fr/file/index/docid/555588/filename/techreport.pdf
+ *
+ * The internal algorithms require transactions to be delivered in causal
+ * order to guarantee stability. E.g. a transaction that removes some text
+ * must be delivered *after* the transaction that adds that text. Any
+ * deviation from this will *not* raise an error, but can lead to
+ * diverging state between peers.
@sccolbert <https://github.com/sccolbert> Regarding this comment: As far
as I read the textfield logic (and saw it in action), any attempt to remove
text that has not yet been added will be discarded, and the text will
remain in the value. Maybe that is because the cemetery attribute of the
textfield metadata is not in use? I'll see if I can create an MWE on
Monday, so we have something specific to discuss, but I thought I would
leave this note for now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#371?email_source=notifications&email_token=AABBQSI64TNDUOFCAKYSNM3P2PGO7A5CNFSM4GUUJXVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3TQ2KY#pullrequestreview-250023211>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABBQSPDQMPW5MYWJ277I2DP2PGO7ANCNFSM4GUUJXVA>
.
|
|
In the current codebase, the cemetery would be added to the metadata.
…On Fri, Jun 14, 2019 at 1:05 PM Chris Colbert ***@***.***> wrote:
Yeah, once you add the cemetery, the causality requirement goes away.
On Fri, Jun 14, 2019 at 12:12 PM Vidar Tonaas Fauske <
***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In packages/datastore/src/datastore.ts
> <#371 (comment)>:
>
> > @@ -32,15 +48,68 @@ import {
> *
> * https://en.wikipedia.org/wiki/Conflict-free_replicated_data_type
> * https://hal.inria.fr/file/index/docid/555588/filename/techreport.pdf
> + *
> + * The internal algorithms require transactions to be delivered in causal
> + * order to guarantee stability. E.g. a transaction that removes some text
> + * must be delivered *after* the transaction that adds that text. Any
> + * deviation from this will *not* raise an error, but can lead to
> + * diverging state between peers.
>
> @sccolbert <https://github.com/sccolbert> Regarding this comment: As far
> as I read the textfield logic (and saw it in action), any attempt to remove
> text that has not yet been added will be discarded, and the text will
> remain in the value. Maybe that is because the cemetery attribute of the
> textfield metadata is not in use? I'll see if I can create an MWE on
> Monday, so we have something specific to discuss, but I thought I would
> leave this note for now.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#371?email_source=notifications&email_token=AABBQSI64TNDUOFCAKYSNM3P2PGO7A5CNFSM4GUUJXVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3TQ2KY#pullrequestreview-250023211>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABBQSPDQMPW5MYWJ277I2DP2PGO7ANCNFSM4GUUJXVA>
> .
>
|
2 participants
Datastoreclass.