TrueCloudLab/frostfs-sdk-go
30
0
Fork 20
Code Issues 29 Pull requests 1 Releases Activity Actions

WIP: [#131] add netmap-aware dialer #196

Closed
olefirenque wants to merge 5 commits from olefirenque/frostfs-sdk-go:173-add_netmap_aware_dialer into master
pull from: olefirenque/frostfs-sdk-go:173-add_netmap_aware_dialer
merge into: TrueCloudLab:master
Conversation 1 Commits 5 Files changed 3 +139 -22
olefirenque commented 2023-11-30 10:14:04 +00:00
Collaborator
Copy link

Refers to #173

Refers to #173
olefirenque added 1 commit 2023-11-30 10:14:05 +00:00
[#131] client: add basic netmap dialer
Some checks failed
Tests and linters / Tests (1.20) (pull_request) Failing after 48s
Details
DCO / DCO (pull_request) Successful in 1m4s
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m8s
Details
Tests and linters / Lint (pull_request) Failing after 1m50s
Details
6ace44f6b5 
Signed-off-by: olefirenque <egor.olefirenko892@gmail.com>
olefirenque added 1 commit 2023-11-30 10:26:34 +00:00
[#131] client: get rid of slices.Equal
Some checks failed
DCO / DCO (pull_request) Successful in 1m0s
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m24s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 1m30s
Details
Tests and linters / Lint (pull_request) Failing after 1m50s
Details
dd8fb59efe 
Signed-off-by: olefirenque <egor.olefirenko892@gmail.com>
fyrchik reviewed 2023-11-30 10:54:59 +00:00
client/client.go
@ -6,2 +6,4 @@
"crypto/tls"
"errors"
"fmt"
netmap "git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/netmap/grpc"
fyrchik commented 2023-11-30 10:30:51 +00:00
Owner
Copy link

stdlib imports should form a separate group, could you move netmap below?

stdlib imports should form a separate group, could you move `netmap` below?
fyrchik commented 2023-11-30 10:30:51 +00:00
Owner
Copy link

stdlib imports should form a separate group, could you move netmap below?

stdlib imports should form a separate group, could you move `netmap` below?
fyrchik commented 2023-11-30 11:09:14 +00:00
Owner
Copy link

Also, we have netmap in the SDK (this repo), can we use it instead of the api package?

Also, we have netmap in the SDK (this repo), can we use it instead of the api package?
olefirenque commented 2023-11-30 11:30:12 +00:00
Author
Collaborator
Copy link

Yes, I used the grpc domain by mistake.
Fixed.

Yes, I used the grpc domain by mistake. Fixed.
client/client.go Outdated
@ -113,0 +119,4 @@
return err
}
isEqualSlices := func(a, b []byte) bool {
fyrchik commented 2023-11-30 10:30:21 +00:00
Owner
Copy link

Use bytes.Equal?

Use `bytes.Equal`?
client/client.go Outdated
@ -113,0 +135,4 @@
nodes := c.prm.NetMap.GetNodes()
for _, node := range nodes {
if isEqualSlices([]byte(u.Host), node.PublicKey) {
return c.Dial(ctx, PrmDial{Endpoint: node.Addresses[0]})
fyrchik commented 2023-11-30 10:54:11 +00:00
Owner
Copy link

This is where we can implement any logic we need. I suggest moving this dial in a separate function, we might want:

  1. Choose addresses from ExternalAddresses attribute
  2. Chose random address.
  3. Handle dial errors (if one address is unavailable, try the next one).

It would be easier to do this in a separate function, in this one we have a simple dial.

This is where we can implement any logic we need. I suggest moving this `dial` in a separate function, we might want: 1. Choose addresses from `ExternalAddresses` attribute 2. Chose _random_ address. 3. Handle dial errors (if one address is unavailable, try the next one). It would be easier to do this in a separate function, in this one we have a simple `dial`.
olefirenque commented 2023-11-30 11:32:54 +00:00
Author
Collaborator
Copy link

Should each option be explicitly defined by client user?

Should each option be explicitly defined by client user?
fyrchik commented 2023-11-30 12:47:55 +00:00
Owner
Copy link

We can discuss. IMO this could be governed with a single parameter, with some reasonable default.

We can discuss. IMO this could be governed with a single parameter, with some reasonable default.
olefirenque added 1 commit 2023-11-30 11:28:53 +00:00
[#131] client: refactor netmap dialer
All checks were successful
DCO / DCO (pull_request) Successful in 1m8s
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m21s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 1m31s
Details
Tests and linters / Lint (pull_request) Successful in 2m51s
Details
cf4677ecb9 
Signed-off-by: olefirenque <egor.olefirenko892@gmail.com>
olefirenque force-pushed 173-add_netmap_aware_dialer from 2b9eaeb2a6 to 7b03c3a6a1 2023-12-07 08:54:42 +00:00 Compare
olefirenque added 1 commit 2023-12-07 09:14:23 +00:00
[#131] client: pass PrmDialOptions to dial call
All checks were successful
DCO / DCO (pull_request) Successful in 1m2s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 1m25s
Details
Tests and linters / Lint (pull_request) Successful in 3m0s
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m1s
Details
aa536d4a4d 
Signed-off-by: olefirenque <egor.olefirenko892@gmail.com>
olefirenque commented 2023-12-07 09:24:17 +00:00
Author
Collaborator
Copy link

Since rpc.Balance is a static function in Client.Dial(...) call, it is hard to reach high coverage of Dial/NetMapDial methods. Would it be suitable to extract rpc call to grpc client for inverting the dependency?

Also I'm concerned about backward compatibility in case of modification prmDial structure (some of field were extracted in the embedding in the last commit). Maybe it would be convenient to have constructors for such types?

Since `rpc.Balance` is a static function in `Client.Dial(...)` call, it is hard to reach high coverage of Dial/NetMapDial methods. Would it be suitable to extract rpc call to grpc client for inverting the dependency? Also I'm concerned about backward compatibility in case of modification prmDial structure (some of field were extracted in the embedding in the last commit). Maybe it would be convenient to have constructors for such types?
fyrchik closed this pull request 2024-08-26 06:33:52 +00:00
All checks were successful
DCO / DCO (pull_request) Successful in 1m2s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 1m25s
Details
Tests and linters / Lint (pull_request) Successful in 3m0s
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 1m1s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
pool

Tasks related to client pool

  
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
good first issue
pool
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
2 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-sdk-go#196
No description provided.
Delete branch "olefirenque/frostfs-sdk-go:173-add_netmap_aware_dialer"

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?