TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 110 Pull requests 5 Projects Releases 2 Activity Actions

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
Conversation 1 Commits 1 Files changed 3 +24 -3
acid-ant commented 2023-05-03 13:05:22 +00:00
Member
Copy link

Close #304

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #304 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
requested reviews from storage-core-committers, storage-core-developers 2023-05-03 13:05:39 +00:00
fyrchik approved these changes 2023-05-03 13:10:49 +00:00
pkg/morph/client/constructor.go Outdated
@ -141,2 +140,2 @@
if err != nil {
return nil, fmt.Errorf("could not create RPC client: %w", err)
for cli.endpoints.curr = range cli.endpoints.list {
cli.client, act, err = cli.newCli(ctx, cli.endpoints.list[cli.endpoints.curr].Address)
fyrchik commented 2023-05-03 13:10:46 +00:00
Owner
Copy link

Can we log errors here? And then print which endpoint was chosen (IMO in INFO level).

Can we log errors here? And then print which endpoint was chosen (IMO in INFO level).
acid-ant commented 2023-05-03 14:14:32 +00:00
Author
Member
Copy link

Please review, in previous version also forgot to exit from loop on success.

Please review, in previous version also forgot to exit from loop on success.
fyrchik marked this conversation as resolved
acid-ant changed title from morph: Iterate over endpoints when create ws client in constructor to WIP: morph: Iterate over endpoints when create ws client in constructor 2023-05-03 14:10:00 +00:00
acid-ant force-pushed bugfix/304-morph-client from 1282c0101d to 5557b4f39d 2023-05-03 14:13:05 +00:00 Compare
acid-ant changed title from WIP: morph: Iterate over endpoints when create ws client in constructor to morph: Iterate over endpoints when create ws client in constructor 2023-05-03 14:13:49 +00:00
requested review from fyrchik 2023-05-03 14:13:51 +00:00
fyrchik reviewed 2023-05-03 16:13:58 +00:00
pkg/morph/client/constructor.go Outdated
@ -143,0 +142,4 @@
cli.client, act, err = cli.newCli(ctx, endpoint.Address)
if err != nil {
err = fmt.Errorf("could not create RPC client for endpoint %s: %w", endpoint.Address, err)
cli.logger.Debug(err.Error())
fyrchik commented 2023-05-03 16:11:42 +00:00
Owner
Copy link

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())`
acid-ant commented 2023-05-04 06:52:20 +00:00
Author
Member
Copy link

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.
acid-ant commented 2023-05-04 07:16:23 +00:00
Author
Member
Copy link

Updated, please review.

Updated, please review.
fyrchik marked this conversation as resolved
pkg/morph/client/constructor.go Outdated
@ -143,0 +143,4 @@
if err != nil {
err = fmt.Errorf("could not create RPC client for endpoint %s: %w", endpoint.Address, err)
cli.logger.Debug(err.Error())
if cli.endpoints.curr == len(cli.endpoints.list)-1 {
fyrchik commented 2023-05-03 16:13:53 +00:00
Owner
Copy link

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 } ```
acid-ant commented 2023-05-04 06:53:25 +00:00
Author
Member
Copy link

Nice suggestion, will implement it.

Nice suggestion, will implement it.
acid-ant commented 2023-05-04 07:16:34 +00:00
Author
Member
Copy link

Updated, please review.

Updated, please review.
fyrchik marked this conversation as resolved
pkg/morph/client/constructor.go Outdated
@ -143,0 +147,4 @@
return nil, err
}
} else {
cli.logger.Info(fmt.Sprintf("created RPC client for endpoint %s", endpoint.Address))
fyrchik commented 2023-05-03 16:12:16 +00:00
Owner
Copy link

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

Again, what about `zap.String("endpoint", endpoint.Address)`?
acid-ant commented 2023-05-04 06:57:34 +00:00
Author
Member
Copy link

Sorry, flashbacks from java :)

Sorry, flashbacks from java :)
acid-ant commented 2023-05-04 07:16:59 +00:00
Author
Member
Copy link

Updated, please review.

Updated, please review.
fyrchik marked this conversation as resolved
acid-ant force-pushed bugfix/304-morph-client from 5557b4f39d to 06112d683f 2023-05-04 07:15:46 +00:00 Compare
fyrchik approved these changes 2023-05-04 13:59:27 +00:00
pkg/morph/client/constructor.go Outdated
@ -53,2 +53,4 @@
)
var (
ErrNoHealthyEndpoint = errors.New("no healthy endpoint")
fyrchik commented 2023-05-04 13:59:06 +00:00
Owner
Copy link

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

Does linter say nothing here? Maybe add some comment to public errors.
acid-ant commented 2023-05-04 14:01:13 +00:00
Author
Member
Copy link

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

Nothing, didn't disable anything related to pre-commit check.
acid-ant commented 2023-05-04 14:16:09 +00:00
Author
Member
Copy link

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

Add more info about ErrNoHealthyEndpoint in help for New(...)
fyrchik reviewed 2023-05-04 14:04:03 +00:00
pkg/morph/client/constructor.go Outdated
@ -143,0 +145,4 @@
for cli.endpoints.curr, endpoint = range cli.endpoints.list {
cli.client, act, err = cli.newCli(ctx, endpoint.Address)
if err != nil {
cli.logger.Debug("could not create RPC client for endpoint",
fyrchik commented 2023-05-04 14:04:03 +00:00
Owner
Copy link

Use constant from internal/logs?

Use constant from `internal/logs`?
acid-ant commented 2023-05-04 14:28:05 +00:00
Author
Member
Copy link

Done

Done
fyrchik marked this conversation as resolved
acid-ant force-pushed bugfix/304-morph-client from 06112d683f to 53abcf25e2 2023-05-04 14:14:15 +00:00 Compare
acid-ant force-pushed bugfix/304-morph-client from 53abcf25e2 to d2f99c4caa 2023-05-04 14:14:43 +00:00 Compare
acid-ant force-pushed bugfix/304-morph-client from d2f99c4caa to 2633bbe13e 2023-05-04 14:28:01 +00:00 Compare
dstepanov-yadro reviewed 2023-05-04 14:38:18 +00:00
pkg/morph/client/constructor.go Outdated
@ -143,0 +148,4 @@
for cli.endpoints.curr, endpoint = range cli.endpoints.list {
cli.client, act, err = cli.newCli(ctx, endpoint.Address)
if err != nil {
cli.logger.Debug(logs.FrostFSIRCouldntCreateRPCClientForEndpoint,
dstepanov-yadro commented 2023-05-04 14:38:18 +00:00
Member
Copy link

Maybe Warn, not Debug?

Maybe Warn, not Debug?
acid-ant commented 2023-05-04 14:57:37 +00:00
Author
Member
Copy link

Why not, updated.

Why not, updated.
dstepanov-yadro approved these changes 2023-05-04 14:38:34 +00:00
acid-ant force-pushed bugfix/304-morph-client from 2633bbe13e to 160bf151e5 2023-05-04 14:57:01 +00:00 Compare
acid-ant scheduled this pull request to auto merge when all checks succeed 2023-05-04 15:02:37 +00:00
fyrchik approved these changes 2023-05-05 07:06:59 +00:00
fyrchik commented 2023-05-05 07:07:24 +00:00
Owner
Copy link

Please, resolve conflicts

Please, resolve conflicts
🚀 1
acid-ant force-pushed bugfix/304-morph-client from 160bf151e5 to cedd07bbc8 2023-05-05 07:07:41 +00:00 Compare
fyrchik merged commit cedd07bbc8 into master 2023-05-05 07:29:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dstepanov-yadro
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#307
No description provided.
Delete branch "acid-ant/frostfs-node:bugfix/304-morph-client"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?