catchup: avoid requesting blocks that aren't needed by the ledger #3089
Conversation
This isn't done for the sake of optimization - it's done so we won't have misleading warning messages in the log file due to frequently canceled contexts.
There was a problem hiding this comment.
I think the fix for the problem is partial.
The new error message will be reported only if the ledger already has the block, but not when the ledger gets the block and cancels the fetch during the innerFetch call.
If the partial fix is good enough, then it will not be necessary to complicate the fix to cover the other cases.
Also, will be nice to test for the error message in a unit test.
Looks good otherwise.
| @@ -218,6 +230,10 @@ func (s *Service) fetchAndWrite(r basics.Round, prevFetchCompleteChan chan bool, | |||
| block, cert, blockDownloadDuration, err := s.innerFetch(r, peer) | |||
|
|
|||
| if err != nil { | |||
| if err == errLedgerAlreadyHasBlock { | |||
There was a problem hiding this comment.
This error is reported only when the ledger already has the block by the time innferFetch first statements are executed.
However, when the innerFetch returns an error because the ledger already has the block, but the ledger had the block after the first innerFetch statements were executed, the error will not be reported as errLedgerAlreadyHasBlock.
Is this the expected behavior?
| @@ -165,7 +177,7 @@ func (s *Service) innerFetch(r basics.Round, peer network.Peer) (blk *bookkeepin | |||
| go func() { | |||
| select { | |||
| case <-stopWaitingForLedgerRound: | |||
| case <-s.ledger.Wait(r): | |||
| case <-ledgerWaitCh: | |||
There was a problem hiding this comment.
Ideally, don't we want errLedgerAlreadyHasBlock returned when ledgerWaitCh fires?
There was a problem hiding this comment.
Sounds like a good idea. I'll make it so.
Codecov Report
@@ Coverage Diff @@
## master #3089 +/- ##
=======================================
Coverage 43.66% 43.67%
=======================================
Files 391 391
Lines 86855 86868 +13
=======================================
+ Hits 37927 37937 +10
- Misses 42895 42898 +3
Partials 6033 6033
Continue to review full report at Codecov.
|
…gorand#3089) Summary avoid requesting blocks that aren't needed by the ledger. This isn't done for the sake of optimization - it's done so we won't have misleading warning messages in the log file due to frequently canceled contexts. Test Plan tested manually.
Summary
avoid requesting blocks that aren't needed by the ledger.
This isn't done for the sake of optimization - it's done so we won't have misleading warning messages in the log file due to frequently canceled contexts.
Test Plan
tested manually.