Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Nov 30, 2023

Related Issues

This will be very useful in context of #10821 and for fully redundant lotus-provider deployments

Proposed Changes

  • Make the multi-node rpc proxy more readable
  • Make the multi-node proxy track the health of backing nodes

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@magik6k magik6k changed the title [wip] feat: cliutil: Improved multinode proxy feat: cliutil: Improved multinode proxy Nov 30, 2023
@magik6k magik6k marked this pull request as ready for review December 1, 2023 08:58
@magik6k magik6k requested a review from a team as a code owner December 1, 2023 08:58
@magik6k magik6k requested review from snadrus and arajasek December 1, 2023 08:58

if bestKnownTipset != nil {
// if we're behind the best tipset, mark as unhealthy
unhealthyProviders[i] = ch.Height() < bestKnownTipset.Height()-maxBehinhBestHealthy
Copy link
Collaborator

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))
Copy link
Collaborator

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)
Copy link
Collaborator

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?

Copy link
Collaborator

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()
Copy link
Collaborator

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")

Copy link
Collaborator

@snadrus snadrus Feb 7, 2024

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
}

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.

2 participants