Refactor getsvc #277

dstepanov-yadro · 2023-04-21T15:23:10Z

dstepanov-yadro commented

2023-04-21 15:23:10 +00:00

The main goal: to make the GET service easier.
The first option is to divide the get service into three services: get, get range, head. But this approach will lead to code duplication. As a result, there will be three complex services.
So I refactored the GET service:

extracted dependencies
resolved cyclic dependencies between structs
renamed types
Also moved errors to separate files.

The main goal: to make the GET service easier. The first option is to divide the get service into three services: get, get range, head. But this approach will lead to code duplication. As a result, there will be three complex services. So I refactored the GET service: - extracted dependencies - resolved cyclic dependencies between structs - renamed types Also moved errors to separate files.

dstepanov-yadro force-pushed object-3606 from fcf8cf19a3 to 3d58994f8a

2023-04-24 09:13:55 +00:00

Compare

dstepanov-yadro changed title from ~~WIP: Refactor getsvc~~ to Refactor getsvc

2023-04-24 11:26:53 +00:00

dstepanov-yadro requested review from storage-core-committers 2023-04-24 11:26:58 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-04-24 11:26:59 +00:00

dstepanov-yadro force-pushed object-3606 from 19bb559f4f to 88469f9859

2023-04-24 11:30:50 +00:00

Compare

fyrchik reviewed 2023-04-24 15:06:01 +00:00

pkg/services/object/get/service.go Outdated

					
				@ -67,2 +20,2 @@

					}

				func New(

					ks keyStorage,

fyrchik commented

2023-04-24 15:03:03 +00:00

Why this change? We use functional options in services and you can do everything with both?

dstepanov-yadro commented

2023-04-24 15:14:14 +00:00

For example, it was not obvious to me which of the dependencies and options were required and which were not.

So New requires mandatory deps, With... - optional.

For example, it was not obvious to me which of the dependencies and options were required and which were not. So `New` requires mandatory deps, `With...` - optional.

dstepanov-yadro commented

2023-04-25 08:00:55 +00:00

Fixed back to Option for logger.

Use this pattern for optional arguments in constructors ...

From https://github.com/uber-go/guide/blob/master/style.md#functional-options

Fixed back to Option for logger. > Use this pattern for optional arguments in constructors ... From https://github.com/uber-go/guide/blob/master/style.md#functional-options

fyrchik commented

2023-04-25 09:59:02 +00:00

Yes, but 5 positional arguments don't look nice either.

Not that I am against the change, but I if we were to commit to a new scheme, I would like to do this atomically across the whole repo. We can create an issue for the discussion.

Yes, but 5 positional arguments don't look nice either. Not that I am against the change, but I if we were to commit to a new scheme, I would like to do this atomically across the whole repo. We can create an issue for the discussion.

ale64bit commented

2023-04-25 10:09:37 +00:00

I would prefer the positional arguments if they are required. They can be packed into a Options struct if needed.
It seems relatively easy to build incorrect objects in other places as well, because of this.

I would prefer the positional arguments if they are required. They can be packed into a `Options` struct if needed. It seems relatively easy to build incorrect objects in other places as well, because of this.

👍 1

fyrchik commented

2023-04-25 10:13:01 +00:00

I like struct, but it seems we do not enforce anything with it either.
However it may be easier to see what arguments are required.

I like struct, but it seems we do not enforce anything with it either. However it may be easier to see what arguments are required.

fyrchik commented

2023-04-25 10:14:47 +00:00

We can try adopting this linter https://github.com/GaijinEntertainment/go-exhaustruct

ale64bit commented

2023-04-25 10:16:50 +00:00

Ofc it's not enforced, especially when having pointer fields. But I guess it's a combination of it being conventional and easier to look at the struct and its doc when calling New, rather than at the multiple option builders which might be spread around.

Ofc it's not enforced, especially when having pointer fields. But I guess it's a combination of it being conventional and easier to look at the struct and its doc when calling `New`, rather than at the multiple option builders which might be spread around.

carpawell commented

2023-04-26 16:18:41 +00:00

we do not enforce anything with it either.

how about a struct that shows a caller that it contains only the required params + validaion inside New (pointers to be sure it is not a default value)? i do not like 5 positional args too

> we do not enforce anything with it either. how about a struct that shows a caller that it contains only the required params + validaion inside `New` (pointers to be sure it is not a default value)? i do not like 5 positional args too

dstepanov-yadro commented

2023-04-27 05:30:43 +00:00

we do not enforce anything with it either.

how about a struct that shows a caller that it contains only the required params + validaion inside New (pointers to be sure it is not a default value)? i do not like 5 positional args too

What's the difference? Instead of 5 arguments, there will be a structure with 5 fields that will most likely be filled in when calling the New method.

For example:

But positional arguments have an advantage: if a new dependency is added to the New method, then with a very high probability the program will not assemble until this dependency is added everywhere.

> > we do not enforce anything with it either. > > how about a struct that shows a caller that it contains only the required params + validaion inside `New` (pointers to be sure it is not a default value)? i do not like 5 positional args too What's the difference? Instead of 5 arguments, there will be a structure with 5 fields that will most likely be filled in when calling the `New` method. For example: - `New` declaration: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/innerring/processors/frostfs/processor.go#L88 - `New` call: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/innerring/initialization.go#L412 But positional arguments have an advantage: if a new dependency is added to the `New` method, then with a very high probability the program will not assemble until this dependency is added everywhere.

acid-ant commented

2023-04-27 06:21:41 +00:00

What's the difference? Instead of 5 arguments, there will be a structure with 5 fields that will most likely be filled in when calling the New method.

For me, it looks unreadable, especially when we provide a bunch of nil vars or empty strings.

> What's the difference? Instead of 5 arguments, there will be a structure with 5 fields that will most likely be filled in when calling the `New` method. For me, it looks unreadable, especially when we provide a bunch of nil vars or empty strings.

dstepanov-yadro commented

2023-04-27 06:54:33 +00:00

For me, it looks unreadable, especially when we provide a bunch of nil vars or empty strings.

It's about required arguments only. If you can pass nil or empty string as an argument value, i think that this argument is not required.

For not required arguments use Option.

> For me, it looks unreadable, especially when we provide a bunch of nil vars or empty strings. It's about required arguments only. If you can pass nil or empty string as an argument value, i think that this argument is not required. For not required arguments use Option.

dstepanov-yadro force-pushed object-3606 from 88469f9859 to b9d557fbff

2023-04-25 07:58:53 +00:00

Compare

carpawell reviewed 2023-04-26 16:28:14 +00:00

pkg/services/object/get/assemble.go Outdated

					
				@ -109,3 +109,1 @@

					w := NewSimpleObjectWriter()

					prm.SetHeaderWriter(w)

					err := exec.svc.Head(ctx, prm)

					err := r.getDetached(ctx, p)