Unify parameters in object services #495

dstepanov-yadro · 2023-07-06T07:08:09Z

dstepanov-yadro commented

2023-07-06 07:08:09 +00:00

Now required dependencies are passed as args, other as options.

Closes #294

Now required dependencies are passed as args, other as options. Closes #294

dstepanov-yadro force-pushed refactor/getsvc_unify_params from df939a090d to 842de037cc

2023-07-06 10:05:54 +00:00

Compare

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 842de037cc to 528bcc8f42

2023-07-06 11:34:40 +00:00

Compare

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 528bcc8f42 to 1c79835600

2023-07-06 11:39:22 +00:00

Compare

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 1c79835600 to 08c4e219f0

2023-07-06 11:59:10 +00:00

Compare

dstepanov-yadro changed title from ~~WIP: Unify parameters in object services~~ to Unify parameters in object services

2023-07-06 12:28:00 +00:00

dstepanov-yadro requested review from storage-core-committers 2023-07-06 12:28:05 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-07-06 12:28:05 +00:00

ale64bit approved these changes 2023-07-10 08:27:41 +00:00

pkg/services/object/acl/acl.go Outdated

					
				@ -38,3 +37,1 @@

				func (c *CheckerPrm) SetValidator(v *eaclSDK.Validator) *CheckerPrm {

					c.validator = v

					return c

				type localStorage struct {

ale64bit commented

2023-07-10 08:25:35 +00:00

why the struct even? just alias it?

why the `struct` even? just alias it?

dstepanov-yadro commented

2023-07-10 08:44:40 +00:00

From here: #495/files

From here: https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/495/files#diff-65b4b70b0453f2a0abd08c354212feeb6a6680d4

ale64bit marked this conversation as resolved

pkg/services/object/acl/acl.go Outdated

					
				@ -46,1 +41,3 @@

				}

				func (s *localStorage) Head(ctx context.Context, addr oid.Address) (*objectSDK.Object, error) {

					if s.ls == nil {

						return nil, io.ErrUnexpectedEOF

ale64bit commented

2023-07-10 08:26:26 +00:00

this might hide a more serious error, no? (i.e. a missing/nil argument somewhere)

dstepanov-yadro commented

2023-07-10 08:49:54 +00:00

again from here: #495/files#diff-65b4b70b0453f2a0abd08c354212feeb6a6680d4

ale64bit marked this conversation as resolved

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

					
				@ -75,0 +72,4 @@

					opts ...Option) *Service {

					c := &cfg{

						log:        &logger.Logger{Logger: zap.L()},

						header:     (*headSvcWrapper)(gs),

ale64bit commented

2023-07-10 08:23:03 +00:00

do we plan to get rid of these casts that scare adults and children alike in another task/PR?

dstepanov-yadro commented

2023-07-10 09:21:22 +00:00

fixed

ale64bit marked this conversation as resolved

aarifullin reviewed 2023-07-10 08:28:12 +00:00

pkg/services/object/acl/v2/service.go Outdated

					
				@ -84,0 +80,4 @@

					cs container.Source,

					opts ...Option) Service {

					cfg := &cfg{

						log:        &logger.Logger{Logger: zap.L()},

aarifullin commented

2023-07-10 08:28:12 +00:00

I think that New should have log log.Logger parameter that can be used for cfg instance creation. This will support dependency inversion.
From the other side - this barely can be helpful because we do not use mocks

I think that `New` should have `log log.Logger` parameter that can be used for `cfg` instance creation. This will support dependency inversion. From the other side - this barely can be helpful because we do not use mocks

dstepanov-yadro commented

2023-07-10 09:23:13 +00:00

logger can be passed with options, because logger is not mandatory and default logger will be used if it is not proovided

`logger` can be passed with options, because logger is not mandatory and default logger will be used if it is not proovided

👍 1

aarifullin reviewed 2023-07-10 08:39:56 +00:00

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

					
				@ -63,0 +62,4 @@

					nk netmap.AnnouncedKeys,

					nst netmap.State,

					opts ...Option) *Service {

					c := &cfg{

aarifullin commented

2023-07-10 08:39:55 +00:00

Here are inevitably many mandatory parameters. Don't you consider to unite some parameters in one struct?

Just for example

type cfgSource struct {
    cs container.Source
    ns netmap.Source
    ms MaxSizeSource
}

Here are inevitably many mandatory parameters. Don't you consider to unite some parameters in one struct? Just for example ```golang type cfgSource struct { cs container.Source ns netmap.Source ms MaxSizeSource } ```

dstepanov-yadro commented

2023-07-10 10:01:56 +00:00

Let's assume that you decided to add a new dependency. If you use a structure with fields, as you suggested, then you can easily forget to initialize one of the fields, and discover it only during execution.

type NewParams struct {
	Dep1Value   Dep1Type
    Dep2Value   Dep2Type
    NewDepValue NewDepType
}


func New(p *NewParams) *Service {
	s := &Service {
    	dep1: p.Dep1Value,
        dep2: p.Dep2Value,
        dep3: p.NewDepValue
    }
}

func createService() *Service {
	return New(&NewParams{
    			Dep1Value: getDep1(),
                Dep2Value: getDep2(),
                // oops, NewDepValue missed
                })
}

In the case of passing by argument, the source code simply will not be assembled until you pass a new dependency.

Let's assume that you decided to add a new dependency. If you use a structure with fields, as you suggested, then you can easily forget to initialize one of the fields, and discover it only during execution. ``` type NewParams struct { Dep1Value Dep1Type Dep2Value Dep2Type NewDepValue NewDepType } func New(p *NewParams) *Service { s := &Service { dep1: p.Dep1Value, dep2: p.Dep2Value, dep3: p.NewDepValue } } func createService() *Service { return New(&NewParams{ Dep1Value: getDep1(), Dep2Value: getDep2(), // oops, NewDepValue missed }) } ``` In the case of passing by argument, the source code simply will not be assembled until you pass a new dependency.

fyrchik commented

2023-07-11 07:31:14 +00:00

Another possibility is having a linter which can check whether all struct fields are filled? Like this:

type Prm struct {
	A // param:obligatory
    B // param:obligatory
    C
}

Another possibility is having a linter which can check whether all struct fields are filled? Like this: ``` type Prm struct { A // param:obligatory B // param:obligatory C } ```

dstepanov-yadro commented

2023-07-11 11:11:29 +00:00

This linter must be smart enough to find all if/else branches.

This linter must be smart enough to find all `if/else` branches.

fyrchik commented

2023-07-11 14:54:59 +00:00

Well, we could enforce providing all values immediately and move all if/else branches above.

Well, we could _enforce_ providing all values immediately and move all if/else branches above.

fyrchik commented

2023-07-11 14:55:13 +00:00

But obviously needs discussion first.

aarifullin approved these changes 2023-07-10 08:40:57 +00:00

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 08c4e219f0 to 0f348270da

2023-07-10 09:20:34 +00:00

Compare

fyrchik commented

2023-07-11 07:28:27 +00:00

For commit messages we had services/put, then putsvc (initiated by you, I believe #277/commits ) and now put service. Can we stay with putsvc, aclsvc etc.?

For commit messages we had `services/put`, then `putsvc` (initiated by you, I believe https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/277/commits ) and now `put service`. Can we stay with `putsvc`, `aclsvc` etc.?

fyrchik referenced this pull request

2023-07-11 07:32:14 +00:00

TestEvacuateShard is flaky #382

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 0f348270da to 35ecdf062d

2023-07-11 11:03:22 +00:00

Compare

dstepanov-yadro commented

2023-07-11 11:10:23 +00:00

For commit messages we had services/put, then putsvc (initiated by you, I believe #277/commits ) and now put service. Can we stay with putsvc, aclsvc etc.?

Done

> For commit messages we had `services/put`, then `putsvc` (initiated by you, I believe https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/277/commits ) and now `put service`. Can we stay with `putsvc`, `aclsvc` etc.? Done

aarifullin approved these changes 2023-07-11 11:32:00 +00:00

fyrchik reviewed 2023-07-11 14:56:21 +00:00

pkg/services/object/acl/v2/service.go Outdated

					
				@ -85,0 +80,4 @@

					acl ACLChecker,

					cs container.Source,

					opts ...Option) Service {

					cfg := &cfg{

fyrchik commented

2023-07-11 14:56:21 +00:00

Can we remove panicOnNil below?

Can we remove `panicOnNil` below?

dstepanov-yadro commented

2023-07-11 15:26:53 +00:00

done

dstepanov-yadro force-pushed refactor/getsvc_unify_params from 35ecdf062d to bb832702e3

2023-07-11 15:26:51 +00:00

Compare

fyrchik approved these changes 2023-07-12 07:33:09 +00:00

fyrchik merged commit 24eb988897 into master

2023-07-12 07:42:12 +00:00

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

4 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#495