prefetcher: improve error codes #3815
Conversation
ledger/ledgercore/error.go
Outdated
| @@ -107,3 +107,14 @@ type ErrNonSequentialBlockEval struct { | |||
| func (err ErrNonSequentialBlockEval) Error() string { | |||
| return fmt.Sprintf("block evaluation for round %d requires sequential evaluation while the latest round is %d", err.EvaluatorRound, err.LatestRound) | |||
| } | |||
|
|
|||
| // GroupTaskError indicates the group index of the unfulfilled resource | |||
| type GroupTaskError struct { | |||
There was a problem hiding this comment.
two requests:
- move the GroupTaskError to the prefetcher package.
- Make GroupTaskError be used with &GroupTaskError, and have the
Unwrap() errorimplemented, so that the caller could use theerrors.Isanderrors.As.
| func (l *prefetcherTestLedger) LookupWithoutRewards(_ basics.Round, addr basics.Address) (ledgercore.AccountData, basics.Round, error) { | ||
| func (l *prefetcherTestLedger) LookupWithoutRewards(rnd basics.Round, addr basics.Address) (ledgercore.AccountData, basics.Round, error) { | ||
| if _, has := l.errorAddress[addr]; has { | ||
| return ledgercore.AccountData{}, l.round, fmt.Errorf("Lookup Error") |
There was a problem hiding this comment.
you might want to use a custom error type here so that you could make errors.Is when testing.
| @@ -71,9 +76,15 @@ func (l *prefetcherTestLedger) LookupApplication(rnd basics.Round, addr basics.A | |||
| return ledgercore.AppResource{}, nil | |||
| } | |||
| func (l *prefetcherTestLedger) LookupAsset(rnd basics.Round, addr basics.Address, aidx basics.AssetIndex) (ledgercore.AssetResource, error) { | |||
| if aidx > 1000000 { | |||
There was a problem hiding this comment.
instead of using a threshold, make a given asset index invalid ( i.e. const errorTriggeringAssetIndex = 1000000 )
|
|
||
| // Test for the GroupIdx in the returned error | ||
| tc := testCases[3] | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
if you're repeating the same test case name, please have this implemented in a separate Test function.
| @@ -408,7 +408,7 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { | |||
| if done.err != nil { | |||
| // if there is an error, report the error to the output channel. | |||
| p.outChan <- LoadedTransactionGroup{ | |||
| Err: done.err, | |||
| Err: ledgercore.GroupTaskError{Err: done.err, GroupIdx: done.groupIdx}, | |||
There was a problem hiding this comment.
With your change, the Err is always either of type ledgercore.GroupTaskError or nil. It sounds as if you should change the Err datatype to be *GroupTaskError.
| tc := testCases[3] | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| errorReceived := false | ||
| groups := make([][]transactions.SignedTxnWithAD, 5) |
There was a problem hiding this comment.
I think that this test would be much more readable if you'll use explicit use cases ( as above ), instead of programmatically modifying the test cases.
Codecov Report
@@ Coverage Diff @@
## master #3815 +/- ##
==========================================
+ Coverage 49.99% 50.01% +0.02%
==========================================
Files 392 393 +1
Lines 68495 68503 +8
==========================================
+ Hits 34242 34265 +23
+ Misses 30497 30487 -10
+ Partials 3756 3751 -5
Continue to review full report at Codecov.
|
Enhance the error reporting in prefetcher
resolves https://github.com/algorand/go-algorand-internal/issues/1921