Add multi beacon client support #876
Conversation
Lint fixes
Set beacon handler for the default beacon process
Fix loading beacon process
client/grpc/client.go
Outdated
| @@ -7,6 +7,8 @@ import ( | |||
| "fmt" | |||
| "time" | |||
|
|
|||
| "github.com/drand/drand/protobuf/common" | |||
There was a problem hiding this comment.
why is this in a separate import block and not next to protobuf/drand below?
| @@ -6,12 +6,14 @@ import ( | |||
| "fmt" | |||
| "sync" | |||
|
|
|||
| "github.com/drand/drand/chain" | |||
go.mod
Outdated
| @@ -58,26 +57,31 @@ require ( | |||
| google.golang.org/protobuf v1.27.1 | |||
| ) | |||
|
|
|||
| require ( | |||
There was a problem hiding this comment.
is it really canonical in golang 1.17 to have 3 require blocks?
There was a problem hiding this comment.
No need to... I've unified all the packages under one require block
| } | ||
|
|
||
| mux := http.NewServeMux() | ||
| //TODO: aggregated bulk round responses. | ||
| mux := chi.NewMux() |
There was a problem hiding this comment.
what are you getting out of chi mux versus the stdlib one?
you're not refactoring the routes to idiomatic chi (mux.With(commonHeaders).Get('/health', handler.Health).
The templating of the chainHashParamKey below feels suspect.
There was a problem hiding this comment.
The main advantage that we make use in chi is the param extract util with chi.URLParam(r, chainHashParamKey).
Since it's compatible with stdlib, we saw no need to change the already established routes.
I can change them to be chi idiomatic as you wrote if that is the case.
http/server.go
Outdated
| chainHashStr = common.DefaultChainHash | ||
| } | ||
|
|
||
| h.state.Lock() |
There was a problem hiding this comment.
This is going to get called on every inbound http client request.
This structure is going to have lock contention.
It at least needs to be an RWMutex because you are only Rlock ing it here, not needing to acquire an exclusive lock.
|
try running |
|
i see: Is this an indication of some edge case where it's possible for multiple beacons to potentially collide? |
No description provided.