-
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: cliutil: Improved multinode proxy #11470
base: master
Are you sure you want to change the base?
Conversation
|
||
if bestKnownTipset != nil { | ||
// if we're behind the best tipset, mark as unhealthy | ||
unhealthyProviders[i] = ch.Height() < bestKnownTipset.Height()-maxBehinhBestHealthy |
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.
Could we do this calculation into a var tmp
and just have healthyLk just wrap this assignment? That would ensure none of the variables are undefined and leaking a lock.
fns = append(fns, rin.MethodByName(field.Name)) | ||
var providerFuncs []reflect.Value | ||
for _, rin := range apiProviders { | ||
providerFuncs = append(providerFuncs, rin.MethodByName(field.Name)) |
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.
.MethodByName() can return a zero value if not given a function. Probably should continue
on that case.
|
||
ctx := args[0].Interface().(context.Context) | ||
|
||
curr := -1 | ||
preferredProvider := new(int) | ||
*preferredProvider = nextHealthyProvider(0) |
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.
Is this interface completely idempotent? i.e. Can we be using different functions of different targets?
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.
It looks like there is special-casing for non-idempotency below. We probably should clean that up to avoid half-commit issues.
} | ||
|
||
for _, out := range outs { | ||
rProxyInternal := reflect.ValueOf(out).Elem() | ||
rOutStruct := reflect.ValueOf(out).Elem() |
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.
The reflection-free way to build this would involve generating the out-struct's functions which call something like nextHealthyProvider() and then do the proxy call, rety & return. The retry could be a static function. This would allow the compiler to ensure all the safe calling of the functions at compile-time. Fewer tests would be needed for code coverage, and coverage would be meaningful (here, you can't know if you have coverage over generated "lines")
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.
ex:
func (g *GeneratedExampleOutStruct) ExampleDefinedInStruct(args any) (res, error) {
for p := range g.HealthyProviders() { // if using the new RangeFunc
res, err := tryCall(args)
if err == errBackendUnreachable {
continue
}
return res, err
}
return nil, ErrNoBackendAvailable
}
Related Issues
This will be very useful in context of #10821 and for fully redundant lotus-provider deployments
Proposed Changes
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, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps