feat(merge-tree): initial implementation of obliterate#12578
Conversation
github-actions
Bot
added
area: dds
Abe27342
left a comment
Abe27342
left a comment
There was a problem hiding this comment.
overall this looks really solid, I have a lot of mostly superficial feedback. I think continuing to chug through fuzz tests on eventual consistency is the right path.
github-actions
Bot
added
area: framework
connorskees
force-pushed
the
feat/obliterate
branch
3 times, most recently
from
871dd3a to
350f42c
Compare
the breaktie change causes perf tests to fail. it seems the change is no longer necessary anyway, so we're free to remove it
…rk into feat/obliterate
Abe27342
left a comment
Abe27342
left a comment
There was a problem hiding this comment.
have got through everything except partial lengths. so far all looks good, only nits (would be OK with addressing all current comments in follow-up PR, and that might be simpler given how awful github UI is with long-running PRs)
| }); | ||
|
|
||
| // todo: a failing obliterate reconnect test. when rebasing the op, | ||
| // the character "C" has been concurrently obliterated, so the reconnect |
There was a problem hiding this comment.
the test setup seems reasonable and like something that's easy to get wrong, but I don't follow the comment here. It looks like we should be rebasing the obliterate op itself, so I'm not sure what's meant by "reconnect position of 'B'" or why it should be 0 or 1.
I would have expected something like:
while rebasing obliterate(0, 2), we:
- rebase position 0 in original context to wherever it is now (still 0)
- rebase position 2 in original context to wherever it is now (will be 3)
of course this is done with segment groups which is a bit different, so i'm not sure I'd write it like that in the code. My point is I'm not sure where the "computed to be 0, rather than 1" comes from.
| export const removeRange: TestOperation = (client: TestClient, opStart: number, opEnd: number) => | ||
| client.removeRangeLocal(opStart, opEnd); | ||
|
|
||
| export const obliterateRange: TestOperation = ( |
There was a problem hiding this comment.
is there some followup work tracking adding this to some of the merge-tree farms?
(this needs to be referenced by the operations field of IMergeTreeOperationRunnerConfig)
There was a problem hiding this comment.
The farms largely fall under AB#3714 (reconnect) and AB#3715 (local references)
|
|
||
| if (minSeq > this.collabWindow.minSeq) { | ||
| this.collabWindow.minSeq = minSeq; | ||
| const firstMoveSeqIdx = this.moveSeqs.findIndex((seq) => seq >= minSeq); |
There was a problem hiding this comment.
could you document on this.moveSeqs that this array is sorted? Otherwise this implementation doesn't work.
You could shortcut the iteration here too, but not really a big deal (the tradeoff for smaller code size might actually be worth it).
There was a problem hiding this comment.
Hm I'm not sure what iteration you mean. findIndex would exit early as soon as it finds a seq gte minSeq, no? I do think this should probably be binary search at some point.
3 participants
Description
Implements the obliterate op.
ado 2115