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

Closed
a-savchuk wants to merge 3 commits from a-savchuk/frostfs-api-go:grpc-call-options into master
4 changed files with 56 additions and 6 deletions

View file

@ -36,7 +36,10 @@ repos:
types: [go]
language: system
- repo: https://github.com/golangci/golangci-lint
rev: v1.60.3
- repo: local
hooks:
- id: golangci-lint
- id: make-lint
name: Run Make Lint
entry: make lint
language: system
pass_filenames: false

View file

@ -8,13 +8,18 @@ PROTOC_OS_VERSION=osx-x86_64
ifeq ($(shell uname), Linux)
PROTOC_OS_VERSION=linux-x86_64
endif
LINT_VERSION ?= 1.60.3
TRUECLOUDLAB_LINT_VERSION ?= 0.0.7
BIN = bin
PROTOBUF_DIR ?= $(abspath $(BIN))/protobuf
PROTOC_DIR ?= $(PROTOBUF_DIR)/protoc-v$(PROTOC_VERSION)
PROTOC_GEN_GO_DIR ?= $(PROTOBUF_DIR)/protoc-gen-go-$(PROTOC_GEN_GO_VERSION)
OUTPUT_LINT_DIR ?= $(abspath $(BIN))/linters
LINT_DIR = $(OUTPUT_LINT_DIR)/golangci-lint-$(LINT_VERSION)-v$(TRUECLOUDLAB_LINT_VERSION)
TMP_DIR := .cache
.PHONY: dep fmts fumpt imports protoc test lint version help $(BIN)/protogen protoc-test
.PHONY: dep fmts fumpt imports protoc test version help $(BIN)/protogen protoc-test
# Pull go dependencies
dep:
@ -92,9 +97,43 @@ test:
@echo "⇒ Running go test"
@GOFLAGS="$(GOFLAGS)" go test ./...
.PHONY: lint-install lint
# Install linters
lint-install:
@rm -rf $(OUTPUT_LINT_DIR)
@mkdir $(OUTPUT_LINT_DIR)
@mkdir -p $(TMP_DIR)
@rm -rf $(TMP_DIR)/linters
@git -c advice.detachedHead=false clone --branch v$(TRUECLOUDLAB_LINT_VERSION) https://git.frostfs.info/TrueCloudLab/linters.git $(TMP_DIR)/linters
@@make -C $(TMP_DIR)/linters lib CGO_ENABLED=1 OUT_DIR=$(OUTPUT_LINT_DIR)
@rm -rf $(TMP_DIR)/linters
@rmdir $(TMP_DIR) 2>/dev/null || true
@CGO_ENABLED=1 GOBIN=$(LINT_DIR) go install -trimpath github.com/golangci/golangci-lint/cmd/golangci-lint@v$(LINT_VERSION)
# Run linters
lint:
@golangci-lint run
@if [ ! -d "$(LINT_DIR)" ]; then \
make lint-install; \
fi
$(LINT_DIR)/golangci-lint run
.PHONY: pre-commit unpre-commit pre-commit-run
# Activate pre-commit hooks
pre-commit:
pre-commit install --hook-type pre-commit
# Deactivate pre-commit hooks
unpre-commit:
pre-commit uninstall --hook-type pre-commit
# Run pre-commit hooks
pre-commit-run:
@pre-commit run --all-files --hook-stage manual
# Print version
version:

View file

@ -55,7 +55,7 @@ func (c *Client) Init(info common.CallMethodInfo, opts ...CallOption) (MessageRe
StreamName: info.Name,
ServerStreams: info.ServerStream(),
ClientStreams: info.ClientStream(),
}, toMethodName(info))
}, toMethodName(info), c.grpcCallOpts...)
Review

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.
Review

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

Lines 14 to 15 in f0fc40e
func SendUnary(cli *Client, info common.CallMethodInfo, req, resp message.Message, opts ...CallOption) error {
rw, err := cli.Init(info, opts...)


Lines 42 to 51 in f0fc40e
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
if err != nil {
cancel()
return nil, err

View file

@ -24,6 +24,7 @@ type cfg struct {
tlsCfg *tls.Config
grpcDialOpts []grpc.DialOption
grpcCallOpts []grpc.CallOption
Review

What is the usecase?

What is the usecase?
Review

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.
Review
  1. 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?
conn Conn
}
@ -127,3 +128,10 @@ func WithGRPCDialOptions(opts []grpc.DialOption) Option {
c.grpcDialOpts = append(c.grpcDialOpts, opts...)
}
}
// WithGRPCDialOptions returns an option to specify grpc.Call.
func WithGRPCCallOptions(opts []grpc.CallOption) Option {
return func(c *cfg) {
c.grpcCallOpts = append(c.grpcCallOpts, opts...)
}
}