WIP: rpc/client: Allow to pass custom grpc.CallOption options #124

a-savchuk · 2024-10-16T15:00:12Z

a-savchuk commented

2024-10-16 15:00:12 +00:00

Signed-off-by: Aleksey Savchuk a.savchuk@yadro.com

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

a-savchuk added 3 commits 2024-10-16 15:00:12 +00:00

[#xx] Makefile: Add lint-install target e7d9c772bd

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

[#xx] Makefile: Add pre-commit related targets 505c2ee13b

Add the following targets:
- `pre-commit` to install pre-commit hooks
- `unpre-commit` to uninstall pre-commit hooks
- `pre-commit-run` to run pre-commit hooks

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

[#xx] rpc/client: Allow to pass custom grpc.CallOption options

Tests and linters / Lint (pull_request) Successful in 50s

Details

DCO action / DCO (pull_request) Failing after 1m9s

Details

Tests and linters / Tests (pull_request) Successful in 1m11s

Details

Tests and linters / Tests with -race (pull_request) Successful in 1m12s

Details

2af8c4a1fc

Those options are used when creating a new grc.ClientStream.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

a-savchuk force-pushed grpc-call-options from 2af8c4a1fc to 3893ca67ac

2024-10-16 15:01:34 +00:00

Compare

a-savchuk referenced this pull request from TrueCloudLab/frostfs-sdk-go

2024-10-16 15:24:15 +00:00

WIP: client: Pass grpc.CallOption options on dial #287

fyrchik reviewed 2024-10-16 15:31:46 +00:00

rpc/client/init.go

					
				@ -56,3 +56,3 @@

						ServerStreams: info.ServerStream(),

						ClientStreams: info.ClientStream(),

					}, toMethodName(info))

					}, toMethodName(info), c.grpcCallOpts...)

fyrchik commented

2024-10-16 15:31:35 +00:00

These are the call options to be passed to every method?
In this case let's use default word everywhere in names (WithDefaultGRPCCallOptions).
An we already have opts ...CallOption in this function, so this is doubly confusing.

These are the call options to be passed to _every_ method? In this case let's use `default` word everywhere in names (`WithDefaultGRPCCallOptions`). An we already have `opts ...CallOption` in this function, so this is doubly confusing.

👍 1

a-savchuk commented

2024-10-16 16:04:34 +00:00

Our CallOptions don't use grpc.CallOption underneath. As I see, you use them only while opening a new connection. I'll consider if we can combine them

	`func SendUnary(cli *Client, info common.CallMethodInfo, req, resp message.Message, opts ...CallOption) error {`
	`rw, err := cli.Init(info, opts...)`

 func (c *Client) Init(info common.CallMethodInfo, opts ...CallOption) (MessageReadWriter, error) {
 	prm := defaultCallParameters()
 	for _, opt := range opts {
 		opt(prm)
 	}
 	if err := c.openGRPCConn(prm.ctx, prm.dialer); err != nil {
 		return nil, err
 	}

Our `CallOption`s don't use `grpc.CallOption` underneath. As I see, you use them only while opening a new connection. I'll consider if we can combine them https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/commit/f0fc40e116d13869bb8a21761749d7666137e15b/rpc/client/flows.go#L14-L15 https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/commit/f0fc40e116d13869bb8a21761749d7666137e15b/rpc/client/init.go#L42-L51

rpc/client/options.go

					
				@ -24,6 +24,7 @@ type cfg struct {

					tlsCfg       *tls.Config

					grpcDialOpts []grpc.DialOption

					grpcCallOpts []grpc.CallOption

fyrchik commented

2024-10-16 15:31:42 +00:00

What is the usecase?

a-savchuk commented

2024-10-16 15:48:37 +00:00

It's WIP yet. I was going to describe entire solution later. In short. I'd like to pass grpc.WaitForReady(true) to a client to make it wait for a deadline.

It's WIP yet. I was going to describe entire solution later. In short. I'd like to pass `grpc.WaitForReady(true)` to a client to make it wait for a deadline.

fyrchik commented

2024-10-16 15:56:55 +00:00

Please, no links to anything unaccessible to a general public.

to make it wait for a deadline

For every call? I think the default behaviour makes sense for some calls. Also, what is the timeout we wait for without this option?

1. Please, no links to anything unaccessible to a general public. 2. >to make it wait for a deadline For _every_ call? I think the default behaviour makes sense for some calls. Also, what is the timeout we wait for without this option?