control: use generics for response wrappers #290

fyrchik · 2023-04-27T10:41:08Z

fyrchik commented

2023-04-27 10:41:08 +00:00

No description provided.

fyrchik force-pushed control-generics from a2f2dfd20a to 531f1cdc40

2023-04-27 10:41:29 +00:00

Compare

fyrchik requested review from storage-core-committers 2023-04-27 10:41:31 +00:00

fyrchik requested review from storage-core-developers 2023-04-27 10:41:31 +00:00

fyrchik force-pushed control-generics from 531f1cdc40 to 83f31348d9

2023-04-27 10:43:24 +00:00

Compare

dstepanov-yadro reviewed 2023-04-27 15:21:19 +00:00

pkg/services/control/convert.go Outdated

					
				@ -28,3 +21,1 @@

					w.m, ok = m.(*HealthCheckResponse)

					if !ok {

						return message.NewUnexpectedMessageType(m, w.m)

				func newResponseWrapper[T any]() *responseWrapper[T] {

dstepanov-yadro commented

2023-04-27 15:20:47 +00:00

Maybe replace any -> grpc.Message? It looks strange if you don't look at grpc.Message definition.

Maybe replace any -> grpc.Message? It looks strange if you don't look at `grpc.Message` definition.

fyrchik commented

2023-04-27 18:20:23 +00:00

Fixed

fyrchik force-pushed control-generics from 83f31348d9 to e86c4ffd01

2023-04-27 18:20:16 +00:00

Compare

fyrchik force-pushed control-generics from e86c4ffd01 to f73ac6e02d

2023-04-28 07:58:04 +00:00

Compare

dstepanov-yadro approved these changes 2023-04-28 08:07:08 +00:00

ale64bit requested changes 2023-04-28 08:08:51 +00:00

pkg/services/control/convert.go

					
				@ -16,3 +16,2 @@

				type healthCheckResponseWrapper struct {

					m *HealthCheckResponse

				type responseWrapper[T grpc.Message] struct {

ale64bit commented

2023-04-28 08:08:13 +00:00

this is kinda duped now:

type responseWrapper[M grpc.Message] struct {

let's move it to a common package and reuse it?

this is kinda duped now: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/ff255212046f508faa637b04d61418bc5bb39fd4/pkg/services/control/ir/convert.go#L17 let's move it to a common package and reuse it?

fyrchik commented

2023-04-28 10:10:30 +00:00

My rule of thumb is to abstract something when we have 3 occurences, we have 2 currently :)
The helper looks simple enough to tolerate some amount of duplication.

My rule of thumb is to abstract something when we have 3 occurences, we have 2 currently :) The helper looks simple enough to tolerate some amount of duplication.

ale64bit commented

2023-04-28 10:37:24 +00:00

As you wish.

IMHO, abstracting it is trivial (move it to a separate package) and it's reasonable to expect that the abstraction will be used more than two places, since it serves to write many wrappers of the same type which are prevalent in the codebase. What's the benefit of tolerating the duplication anyway?

Could you elaborate how you came to the constant 3 in your rule of thumb? I'm curious about the rationale behind it.

As you wish. IMHO, abstracting it is trivial (move it to a separate package) and it's reasonable to expect that the abstraction will be used more than two places, since it serves to write many wrappers of the same type which are prevalent in the codebase. What's the benefit of tolerating the duplication anyway? Could you elaborate how you came to the constant 3 in your rule of thumb? I'm curious about the rationale behind it.

fyrchik commented

2023-04-28 10:40:32 +00:00

We will simplify the api-go itself in the near future, as @dstepanov-yadro noticed, these wrappers look like a kludge.

ale64bit marked this conversation as resolved

ale64bit approved these changes 2023-04-28 11:04:41 +00:00

fyrchik merged commit f73ac6e02d into master

2023-04-28 13:59:44 +00:00

fyrchik referenced this pull request from a commit

2023-04-28 13:59:44 +00:00

[#290] control: Use generics for response wrappers

fyrchik deleted branch control-generics

2023-04-28 13:59:44 +00:00