Skip to content

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Mar 27, 2022

Batch manager updates

I believe I've found an issue in the way the batch manager and the batch processors interact
Because the messages stay in ready state while they are being dispatched, the readPage loop will still see all messages that are dispatched

Currently it attempts to re-dispatch them when it's told to rewinds, and relies on the processor to de-dup those messages
It all works from a consistency perspective, but with the pattern of workload from the long-run it's a little inefficient in terms of how many rewinds and re-processing we do. It seems this can reach the point the batch manager can get jammed up trying to do those re-dispatches to the processor, while the processor is busy flushing what it's got

This PR simplifies the logic significantly, by preventing duplicate dispatches in the batch manager.

The manager keeps a map of all in-flight dispatched sequences to processors, and the processors call back to the manager when they have flushed a batch (so the messages will no longer turn up in readPage queries).

Fix to private blobs

The private message batch dispatcher, was only sending the first blob in a batch. Meaning other messages with blobs in the batch would never be confirmed, because the blobs never arrived.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #633 (4062d62) into main (53ecb34) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #633   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          311       311           
  Lines        18842     18860   +18     
=========================================
+ Hits         18842     18860   +18     
Impacted Files Coverage Δ
internal/batch/batch_manager.go 100.00% <100.00%> (ø)
internal/batch/batch_processor.go 100.00% <100.00%> (ø)
internal/config/config.go 100.00% <100.00%> (ø)
internal/privatemessaging/privatemessaging.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ecb34...4062d62. Read the comment docs.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst changed the title Update batch manager dispatch to track inflight Update batch manager dispatch to track inflight and fix private blobs Mar 27, 2022
Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with this part of the code, but this looks good to me. Also one minor spelling thing in a comment, but not a big deal.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst merged commit 10c69ad into hyperledger:main Mar 27, 2022
@peterbroadhurst peterbroadhurst deleted the batch-inflight branch March 27, 2022 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants