Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the BatchStreamer where fallbackBatchPos was incorrectly initialized, causing Reset() to skip a batch. It also updates the skipPos logic to ensure that when the buffer is full, the streamer rewinds to the block immediately preceding the dropped batch, preventing data loss. Regression tests were added to verify the initialization fix and the rewind behavior. I have no feedback to provide.
0eed372 to
61ce29d
Compare
Ayiga
left a comment
There was a problem hiding this comment.
I was initially confused by the underflow protection code. But that was just me misreading it. (I think I was reading it as a variable assigning to itself, instead of the temporary variable)
Everything looks to be good to me.
(Be careful with committing those git diff artifacts :P)
Closes https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213432243074587 and
https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213432243074595
This PR:
Bug fix — fallbackBatchPos initialization (NewEspressoStreamer):
fallbackBatchPoswas incorrectly initialised tooriginBatchPos + 1, so aReset()immediately after construction would advance BatchPos by an extra step. Fixed tooriginBatchPos(no +1), matching its documented semantics as "last safe batch".Corresponds to ticket https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213432243074595
New test — TestResetAfterNewDoesNotSkipBatch:
Regression test for the fallbackBatchPos init bug — verifies BatchPos stays at originBatchPos + 1 after a Reset() immediately following construction.
Bug fix — off-by-one in skipPos rewind (fetchHotShotRange):
When a batch failed with ErrAtCapacity, skipPos was set to the range's start. After rewinding (hotShotPos = skipPos), the next scan computed start + 1, permanently skipping the batch at block start. Fixed by tracking the exact failing blockPos and recording skipPos = blockPos - 1, so the rewind correctly re-scans from blockPos.
Corresponds to ticket https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213432243074587
New test — rewind rescans the first block of the range where overflow began:
Regression test for the skipPos off-by-one — fills the buffer to exactly capacity, then places an overflow batch at the very first block of the next range. Verifies the batch is recoverable after the rewind.
Rename — namespaceRangeTransactions / namespaceTransaction:
Each element of the slice is a HotShot block (containing namespace transactions), not a transaction itself. Renamed to hotShotBlocks / namespaceBlock and updated the log key to numHotShotBlocks.
Key places to review:
Changes in
op_streamer.go.How to test this PR:
just test