Workaround WAVE-446 #24
Conversation
| List<TransformedWaveletDelta> singleDeltaList = new ArrayList<TransformedWaveletDelta>(1); | ||
| singleDeltaList.add(delta); | ||
| sendUpdate(waveletName, singleDeltaList, null); | ||
| } |
There was a problem hiding this comment.
Looks like the bracket could use formatting.
There was a problem hiding this comment.
Oh, thanks, always struggling with formatters :(
| // | ||
| // Workaround for WAVE-446 ([email protected]) | ||
| // | ||
| for (TransformedWaveletDelta delta: filteredDeltas) { |
There was a problem hiding this comment.
Can we extract the code into a separate method with a descriptive name instead of putting a comment in the code?
There was a problem hiding this comment.
Sure, that is neater
| // Workaround for WAVE-446 ([email protected]) | ||
| // | ||
| for (TransformedWaveletDelta delta: filteredDeltas) { | ||
| List<TransformedWaveletDelta> singleDeltaList = new ArrayList<TransformedWaveletDelta>(1); |
There was a problem hiding this comment.
Can we use SingletonList instead?
In fact, it looks like it is enough to partition the filteredDeltas into sublists with contiguous deltas and send only those sublists separately.
We can split with something like this (didn't test this yet):
@VisibleForTesting
List<List<TransformedWaveletDelta>> splitToContiguousParts(List<TransformedWaveletDelta>
filteredDeltas) {
Stack<TransformedWaveletDelta> stack = new Stack<>();
Pair<List<List<TransformedWaveletDelta>>, Stack<TransformedWaveletDelta>> identity =
Pair.of(Lists.newArrayList(), stack);
Pair<List<List<TransformedWaveletDelta>>, Stack<TransformedWaveletDelta>> result =
filteredDeltas.stream().reduce(identity, (u, t) -> {
Stack<TransformedWaveletDelta> stackBuffer = u.getSecond();
if (stackBuffer.isEmpty() ||
stackBuffer.peek().getResultingVersion().getVersion() + 1 == t.getResultingVersion().getVersion()) {
stackBuffer.push(t);
} else {
Collections.reverse(stackBuffer);
u.getFirst().add(Lists.newArrayList(stackBuffer));
stackBuffer.clear();
stackBuffer.push(t);
}
return u;
}, (a, b) -> b);
List<List<TransformedWaveletDelta>> out = result.getFirst();
Stack<TransformedWaveletDelta> stackBuffer = result.getSecond();
Collections.reverse(stackBuffer);
out.add(Lists.newArrayList(stackBuffer));
return out;
}There was a problem hiding this comment.
Yes, you are right, pack and send contiguous deltas works. I wonder whether to add that split function is worthwhile as mere workaround because first, as far as I see in the logs during debug, it is not very common to send more than 2 or 3 deltas at once. Secondly, I think the actual issue is in the client side (or protocol's message): since each received delta message doesn't have both start and end version, the deserialize method can't infer the right ones if they are not contiguous.
There was a problem hiding this comment.
I guess let's just implement a naive splitter that packs every delta into a separate list. I think the better solution would be to take the client-server implementation from the wiab pro. They totally rewrote and improved it.
There was a problem hiding this comment.
Maybe I'm wrong but this sounds to me a common "perfect is the enemy of good" situation. What about if we merge this PR, while we think/work in your proposal of using the wiab pro code?
There was a problem hiding this comment.
Sure, that's what I intended to say, probably I wasn't clear.
|
Thanks indeed Pablo for taking care of this important bug. |
|
is this approved? |
|
I didn't test the last commit but it LGTM |
Workaround for critical bug in client/server protocol implementation
See https://issues.apache.org/jira/browse/WAVE-446