fix replication requests exceeding maxRoundWindow + splitting resends#431
fix replication requests exceeding maxRoundWindow + splitting resends#431jadalsmail wants to merge 11 commits into
Conversation
jadalsmail
force-pushed
the
fix-replication-request-splitting
branch
from
7261a83 to
a70872d
Compare
yacovm
left a comment
yacovm
left a comment
There was a problem hiding this comment.
I also want to review this change before it gets merged so please don't merge it yet
Can we please have a more descriptive and elaborate PR description? |
jadalsmail
force-pushed
the
fix-replication-request-splitting
branch
2 times, most recently
from
b3fa884 to
043ac1e
Compare
| ReplicationEnabled: true, | ||
| // twice the responder's window: the timed-out sequences span more | ||
| // than a responder is willing to server in a single request | ||
| MaxRoundWindow: startSeq, |
There was a problem hiding this comment.
Ideally we would reproduce the problem without specifying unique configuration such as modifying the max round window.
There was a problem hiding this comment.
I agree, but here's the thing, the bug was reproducible with default config before #432, but that fix removed its trigger (outstanding requests can no longer exceed the responder limit when windows match). Since this PR merges both send paths into one function, the aggregation half can't fire on its own anymore. The window override recreates the same trigger condition #432 eliminated, so the test actually exercises the drop-and-wedge path and proves the new code survives it.
There was a problem hiding this comment.
I agree, but here's the thing, the bug was reproducible with default config before #432, but that fix removed its trigger (outstanding requests can no longer exceed the responder limit when windows match). Since this PR merges both send paths into one function, the aggregation half can't fire on its own anymore. The window override recreates the same trigger condition #432 eliminated, so the test actually exercises the drop-and-wedge path and proves the new code survives it.
Can you explain what you mean by:
drop-and-wedge
?
There was a problem hiding this comment.
Drop the requests that are coming from the peer nodes.
wedge: keeping the node behind (lagging forever)
There was a problem hiding this comment.
Since this PR merges both send paths into one function, the aggregation half can't fire on its own anymore.
I do not expect to create a scenario that is influenced by existing production code changes from this PR.
Is it not possible to create a test with two epochs or more that if I completely revert all production code changes and leave the test intact, the test will fail because of the reason the PR claims to be the issue?
3 participants
Motivation:
In resendReplicationRequests, timed-out sequences were compressed into contiguous segments and each segment was sent whole to a single node, which can be larger than maxRoundWindow. Thus, nodes drop such requests without responding, so a lagging node would resend the same oversized request forever and never catch up.
initial requests and timed-out retries now share a single send path. A new function, BatchSequences, which distributes seqs as evenly as possible among nodes, returning one batch per node share. Every batch contains at most maxSize sequences.
We also introduce a new variable, MaxRoundRequests, which is the max number of rounds or sequences that fit in one replication request. Every batch is capped at this size by construction. It is set to 10 to match DefaultMaxRoundWindow
Removed: CompressSequences, DistributeSequenceRequests, and the Segment type (plus their tests). the segment/range model they implemented is replaced by batching, and requests may now contain non-contiguous sequences (responders sort incoming requests).
Also, unit test + table tests (for BatchSequences) for testing this functionality were added.