Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optional insert operation #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dmitryuv
Copy link

Initial version of the "optional insert" operation. The best name I've found is "oin" which means Object Insert if Null.

The goal is to solve situation when both clients to start offline with empty doc and want to collaborate on subobject with known name. Without this operation, last client that syncs to the server will overwrite everything first client did.

Basic transform rules:

(oin, oin') -> oin'
(oin', oin) -> oin
(oin, oi) -> noop
(oi, oin) -> od, oi

Composition rules:

oi+oin -> oi
oin + oi -> od, oi
oin' + oin -> oin'
od + oin -> od, oi

Inversion:

invert(oin) -> od 

The operation will be deterministic only with sane usage (including invert rule). For example, everyone should try to init document with the same initial value. Everyone should not try to shot operation if he knows for sure that object exists.

@dmitryuv
Copy link
Author

I think i forgot to add a test for applying an op, will add tomorrow

@josephg
Copy link
Member

josephg commented Apr 25, 2014

At a glance it looks good, but you're right we also need tests for applying. Also you need to update the randomizer to emit oin components sometimes. Its an extremely effective bug finding machine.

@nornagon can you take a quick look at this?

@nornagon
Copy link
Contributor

This seems a bit hacky, would a potential alternative solution be to have the document start with an initial value? so if all clients do 'open or create doc with {default}' where 'default' is the same across all clients, then they can all submit ops to the doc without having the initial od+oi?

@josephg
Copy link
Member

josephg commented Apr 25, 2014

What, like a default document template for each collection? We already support the ability to initialize a document with data. When you create a document you can give it initial content.

But you can't initialize everything - eg, if I want an object which has counts of the number of times different actions have happened - eg, {swim:5, run:2}, I would have to know all the actions ahead of time. If two clients try and {p:['bounce'], oi:1} at the same time they'll clobber each other's initialization and you'll miss a count. With setNull you could do [{p:['bounce'], oin:0}, {p:['bounce'], na:1}] and it'll work.

@josephg
Copy link
Member

josephg commented Apr 25, 2014

... A way to think about it which avoids the hacky feeling is that oi: has the semantics of last writer wins. oin is the same but has the semantics of whoever makes it not null wins. Its much more meek.

Maybe a cleaner way to implement it would be a variant of od: oi: that did first-writer-wins instead of last-writer-wins. Then:

  • The value currently being null is unimportant
  • Semantics in apply() are identical to the od: oi: case
  • In transform, the first op (server's op) always wins instead of the last (client's)

@josephg
Copy link
Member

josephg commented Apr 25, 2014

... In that case, setNull() would be if(currently null) { setFWW(value); }. If someone else set it before you did, their change will win. The difference is that if someone sets it then it gets deleted again, it'll be null again. But I think thats ok.

@dmitryuv
Copy link
Author

@josephg if I understand you correctly, then if I do sequence like

setFWW(value) 
inc(value.num)

Then if someone inserted before me, my value will be removed. But also tranform() will remove all operations on removed value, and I feel I'll need a real bad hack to avoid this.

@dmitryuv
Copy link
Author

small update: actually "apply" test already there, on line 330, just forgot I added it :)
Right now I'm slamming my head about adding "oin" op to randomizer. The tricky thing is that if clients want to use it, they both have to agree on inserting initial value this way. Randomizer generates random OP for each client and do not communicate about strategy, so for ex. if first one decides to do "oin", and another just "oi" on the same key, everything will be broken.

@josephg
Copy link
Member

josephg commented Jun 24, 2014

Well whats the right outcome if one client does oin and another client does oi? It sounds like regardless of what happens, the oi result should be applied.

@dmitryuv
Copy link
Author

Uhh, that was 2 months ago. After your comment I've returned to my code and it took me half day to recall the problem and finally fix the test :)

@dmitryuv
Copy link
Author

something fails on invert with new seed, have to figure it out

@josephg
Copy link
Member

josephg commented Jun 27, 2014

:) I recommend cranking up the randomiser iterations while you're messing
around. I'm looking forward to using it!

-J

see PR discussion for details
@dmitryuv
Copy link
Author

Ok I fully recalled the problem with randomizer. The idea behind oin operation is that all clients should agree that they're doing initialisation of the key in the same manner. That is:

  1. They should do oin only if they don't have value for the key in their local snapshot
  2. They should know and supply the same initialisation value for oin

With randomizer, I'm deciding whatever I need to create oi or oin operation based on random seed value and for the same keys I can decide to do different operations. The result is that:

  1. Client A performs [{p:['key'],oi:{val:X}}],[{p:['key'],od:{val:X}}]
  2. Client B performs [{p:['key'],oin:{val:Y}}],[{p:['key'],od:{val:Y}}]

Now when you'll try to transform Client B operations against Client A, you'll get:

  1. oin will be dropped since there is a existing key with {val:X}
  2. object delete will now reference wrong value {val:Y} while for Client A it's {val:X}
  3. While final snapshot is still consistent across clients, everything breaks apart on invert operation

The way to fix it is to run oi and oin tests separately, always doing either one or another operation.

@josephg
Copy link
Member

josephg commented Jul 28, 2014

Well, we can do better than that. Instead of quietly breaking, lets define the semantics of what happens if two clients both execute those operations at the same time. In this case, I think its simpler to think of oin: as a first-writer-wins edit and oi: as last-writer-wins. In that case, the oi:X will overwrite the oin:Y value. I think this is a superset of the functionality you're after.

As for hacking the tests, those operations will be interspersed in any given JSON object - if we hack the randomizer to only use one kind of operation at a time, it'll stop being able to find some bugs.

@dmitryuv
Copy link
Author

@josephg hope you're still with me. I haven't had time to think more about this yet, but yesterday fuzzer found a problem which I'm not sure how to deal with.

test case is simple:

expect(json0.transform([{'p': [], 'oin': 1}, {'p': [], 'od': 1}], [{'p':[], 'oi':2}], 'left'), equals([]));
expect(json0.transform([{'p': [], 'oin': 1}, {'p': [], 'od': 1}], [{'p':[], 'oi':2}], 'right'), equals([{'p': [], 'oi': 2}]));
  1. I hope that my assumptions about the result are correct
  2. First client does optional insert and then decides to remove the object. Following original implementation, oin will be dropped in the left transform, but od remains in place, breaking apply and invert operations. Same problem for the right transform.

The problem is because later ops working on the same path need to be aware that oin was dropped. Or something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants