-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: message: Add retries to mpool push message from lotus miner #9177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks good, 2 small comments on the test
itests/retry_test.go
Outdated
"github.com/filecoin-project/lotus/lib/retry" | ||
) | ||
|
||
func TestRetryErrorIsInTrue(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itests/
is really just for integration tests which require running test nodes. This test can be in lib/retry/retry_test.go
; CI will still run that with other unit tests
itests/retry_test.go
Outdated
|
||
func TestRetryErrorIsInTrue(t *testing.T) { | ||
errorsToRetry := []error{&jsonrpc.RPCConnectionError{}} | ||
require.True(t, retry.ErrorIsIn(&jsonrpc.RPCConnectionError{}, errorsToRetry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to also test a wrapped error - something like require.True(t, retry.ErrorIsIn(xerrors.Errorf("wrapped: %w", &jsonrpc.RPCConnectionError{}), errorsToRetry))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
There also are some other Go 1.17 references that possibly need updating; this is what seems important on master:
|
Related Issues
#9171
Proposed Changes
Add retries to mpool push message API in case of unavailability of the lotus chain node
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea
: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps