Skip to content
This repository was archived by the owner on Feb 8, 2019. It is now read-only.

Workaround WAVE-446 #24

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

Workaround WAVE-446 #24

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e35f543
Workaround WAVE-443
pablojan May 1, 2017
1199e35
Implement WAVE-446 as separated method.
pablojan May 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion wave/src/main/java/org/waveprotocol/box/server/frontend/WaveViewSubscription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.waveprotocol.wave.model.version.HashedVersion;
import org.waveprotocol.wave.util.logging.Log;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -154,7 +155,15 @@ public synchronized void submitResponse(WaveletName waveletName, HashedVersion v
// Forward any queued deltas.
List<TransformedWaveletDelta> filteredDeltas = filterOwnDeltas(state.heldBackDeltas, state);
if (!filteredDeltas.isEmpty()) {
sendUpdate(waveletName, filteredDeltas, null);
//
// Workaround for WAVE-446 ([email protected])
//
for (TransformedWaveletDelta delta: filteredDeltas) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract the code into a separate method with a descriptive name instead of putting a comment in the code?

Copy link
Contributor Author

@pablojan pablojan May 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that is neater

List<TransformedWaveletDelta> singleDeltaList = new ArrayList<TransformedWaveletDelta>(1);
Copy link
Contributor

@vega113 vega113 May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's what I intended to say, probably I wasn't clear.

singleDeltaList.add(delta);
sendUpdate(waveletName, singleDeltaList, null);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the bracket could use formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks, always struggling with formatters :(


}
state.heldBackDeltas.clear();
}
Expand Down