client: `(*Client).Dial` seems to be redundant #308

New issue

Open

opened 2024-12-09 08:39:13 +00:00 by a-savchuk · 1 comment

a-savchuk commented

2024-12-09 08:39:13 +00:00

Member

After incorporating the approach from #304, I think we should consider whether we still need the Dial method.

	`_, err := rpc.Balance(&c.c, new(v2accounting.BalanceRequest),`
	`client.WithContext(ctx),`
	`)`

Currently, we use this method to check if the target node is alive before making a request. As far as I know, it was another way to cope with the lack of distinction between the dialing and streaming steps. After #304 is merged, the previous approach seems to be redundant.

Examples of its usage in frostfs-node:

	`c.Init(prmInit)`
	`err := c.Dial(ctx, prmDial)`

	`c.Init(prmInit)`

	`if err := c.Dial(ctx, prmDial); err != nil {`

Describe the solution you'd like

Get rid of (*Client).Dial

Describe alternatives you've considered

Keep things as they are

## Is your feature request related to a problem? Please describe. After incorporating the approach from https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/304, I think we should consider whether we still need the [Dial](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/81c423e7094d870f7e38e5ccca8e4690dfe89699/client/client.go#L82-L128) method. https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/81c423e7094d870f7e38e5ccca8e4690dfe89699/client/client.go#L105-L107 Currently, we use this method to check if the target node is alive before making a request. As far as I know, it was another way to cope with the lack of distinction between the dialing and streaming steps. After https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/304 is merged, the previous approach seems to be redundant. Examples of its usage in `frostfs-node`: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/7e542906ef9085a792e5ec5f7185bfdd9acecedb/pkg/network/cache/multi.go#L92-L93 https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/7e542906ef9085a792e5ec5f7185bfdd9acecedb/cmd/frostfs-cli/internal/client/sdk.go#L74-L76 ## Describe the solution you'd like Get rid of `(*Client).Dial` ## Describe alternatives you've considered Keep things as they are

a-savchuk added the

discussion

label 2024-12-09 08:39:13 +00:00

a-savchuk changed title from ~~client: (c *Client) Dial seems to be redundant~~ to client: (*Client) Dial seems to be redundant

2024-12-09 08:39:22 +00:00

a-savchuk changed title from ~~client: (*Client) Dial seems to be redundant~~ to client: (*Client).Dial seems to be redundant

2024-12-09 08:39:31 +00:00

fyrchik added the

refactoring

label 2024-12-09 09:50:54 +00:00

aarifullin commented

2024-12-10 11:28:03 +00:00

Member

Dial is used in frostfs-node's multiclient within createForAddress.
The problem is that you can't just remove healthchecking part (equals to rpc.Balance(...)) from Dial. This may break the client caching within multiclient as you will "healthcheck" it in lazy manner. Here it comes:

No Dial -> createForAddress always returns a client
OK client -> no c.lastAttempt = time.Now(). Although the problem may hide and get disclosed after any RPC-call
no c.lastAttempt = time.Now() -> no errRecentlyFailed (at all ???)
clients will be invalidated only after RPC invocation -> may hit the performance

So, we should consider this point before erasing Dial

`Dial` is used in frostfs-node's multiclient within [createForAddress](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/network/cache/multi.go#L92-L93). The problem is that you can't just remove healthchecking part (equals to `rpc.Balance(...)`) from `Dial`. This may break the client [caching](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/network/cache/multi.go#L334-L383) within multiclient as you will "healthcheck" it in lazy manner. Here it comes: - No `Dial` -> `createForAddress` always returns a client - OK client -> no `c.lastAttempt = time.Now()`. Although the problem may hide and get disclosed after any RPC-call - no `c.lastAttempt = time.Now()` -> no `errRecentlyFailed` (at all **???**) - clients will be invalidated only after RPC invocation -> may hit the performance So, we should consider this point before erasing `Dial`

👍 1