morph: Iterate over endpoints when create ws client in constructor #307

Merged
fyrchik merged 1 commit from acid-ant/frostfs-node:bugfix/304-morph-client into master 2023-05-05 07:29:47 +00:00
3 changed files with 24 additions and 3 deletions

View file

@ -13,6 +13,7 @@ Changelog for FrostFS Node
- Read config files from dir even if config file not provided via `--config` for node (#238)
- Notary requests parsing according to `neo-go`'s updates (#268)
- Tree service panic in its internal client cache (#322)
- Iterate over endpoints when create ws client in morph's constructor (#304)
### Removed
### Updated

View file

@ -413,6 +413,8 @@ const (
FrostFSIRInternalError = "internal error" // Info in ../node/cmd/frostfs-ir/main.go
FrostFSIRCouldNotShutdownHTTPServer = "could not shutdown HTTP server" // Debug in ../node/cmd/frostfs-ir/main.go
FrostFSIRApplicationStopped = "application stopped" // Info in ../node/cmd/frostfs-ir/main.go
FrostFSIRCouldntCreateRPCClientForEndpoint = "could not create RPC client for endpoint" // Debug in ../node/pkg/morph/client/constructor.go
FrostFSIRCreatedRPCClientForEndpoint = "created RPC client for endpoint" // Info in ../node/pkg/morph/client/constructor.go
FrostFSNodeCouldNotReadCertificateFromFile = "could not read certificate from file" // Error in ../node/cmd/frostfs-node/grpc.go
FrostFSNodeCantListenGRPCEndpoint = "can't listen gRPC endpoint" // Error in ../node/cmd/frostfs-node/grpc.go
FrostFSNodeStopListeningGRPCEndpoint = "stop listening gRPC endpoint" // Info in ../node/cmd/frostfs-node/grpc.go

View file

@ -7,6 +7,7 @@ import (
"sync"
"time"
"git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/logger"
lru "github.com/hashicorp/golang-lru/v2"
"github.com/nspcc-dev/neo-go/pkg/core/block"
@ -52,6 +53,10 @@ const (
defaultWaitInterval = 500 * time.Millisecond
)
var (

Does linter say nothing here?
Maybe add some comment to public errors.

Does linter say nothing here? Maybe add some comment to public errors.

Nothing, didn't disable anything related to pre-commit check.

Nothing, didn't disable anything related to pre-commit check.

Add more info about ErrNoHealthyEndpoint in help for New(...)

Add more info about ErrNoHealthyEndpoint in help for New(...)
ErrNoHealthyEndpoint = errors.New("no healthy endpoint")
)
func defaultConfig() *cfg {
return &cfg{
dialTimeout: defaultDialTimeout,
@ -80,6 +85,8 @@ func defaultConfig() *cfg {
// If desired option satisfies the default value, it can be omitted.
// If multiple options of the same config value are supplied,
// the option with the highest index in the arguments will be used.
// If the list of endpoints provided - uses first alive.
// If there are no healthy endpoint - returns ErrNoHealthyEndpoint.
func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, error) {
if key == nil {
panic("empty private key")
@ -137,9 +144,20 @@ func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, er
return nil, fmt.Errorf("could not create RPC actor: %w", err)
}
fyrchik marked this conversation as resolved Outdated

We usually log the message and use zap.Error, why have you decided to Debug(err.Error())

We usually log the message and use `zap.Error`, why have you decided to `Debug(err.Error())`

Because in this case, error already contains context. Also, to avoid duplication, cause in the worth case we need to return error.

Because in this case, error already contains context. Also, to avoid duplication, cause in the worth case we need to return error.

Updated, please review.

Updated, please review.
} else {
fyrchik marked this conversation as resolved Outdated

Can we move this line out of the loop?
The error could be defined separately -- it is not the last, but e.g. ErrNoHealthyEndpoint

var cli *Client
for { ... }
if cli == nil {
  return nil, ErrNoHealthyEndpoint
}
Can we move this line out of the loop? The error could be defined separately -- it is not the last, but e.g. `ErrNoHealthyEndpoint` ``` var cli *Client for { ... } if cli == nil { return nil, ErrNoHealthyEndpoint } ```

Nice suggestion, will implement it.

Nice suggestion, will implement it.

Updated, please review.

Updated, please review.
cli.client, act, err = cli.newCli(ctx, cli.endpoints.list[0].Address)
if err != nil {
return nil, fmt.Errorf("could not create RPC client: %w", err)
var endpoint Endpoint
for cli.endpoints.curr, endpoint = range cli.endpoints.list {
fyrchik marked this conversation as resolved Outdated

Use constant from internal/logs?

Use constant from `internal/logs`?

Done

Done
cli.client, act, err = cli.newCli(ctx, endpoint.Address)
if err != nil {
fyrchik marked this conversation as resolved Outdated

Again, what about zap.String("endpoint", endpoint.Address)?

Again, what about `zap.String("endpoint", endpoint.Address)`?

Sorry, flashbacks from java :)

Sorry, flashbacks from java :)

Updated, please review.

Updated, please review.
cli.logger.Warn(logs.FrostFSIRCouldntCreateRPCClientForEndpoint,

Maybe Warn, not Debug?

Maybe Warn, not Debug?

Why not, updated.

Why not, updated.
zap.Error(err), zap.String("endpoint", endpoint.Address))
} else {
cli.logger.Info(logs.FrostFSIRCreatedRPCClientForEndpoint,
zap.String("endpoint", endpoint.Address))
break
}
}
if cli.client == nil {
return nil, ErrNoHealthyEndpoint
}
}
cli.setActor(act)