TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 5 Projects Releases 1 Activity Actions

Refactor get service #193

Merged
fyrchik merged 7 commits from dstepanov-yadro/frostfs-node:refactoring/OBJECT-3610_getsvc into master 2023-04-05 14:38:49 +00:00
Conversation 1 Commits 7 Files changed 14 +29 -25
4 changed files with 29 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 7dbeb09b94 - Show all commits

11
pkg/services/object/get/v2/get_forwarder.go
View file

@ -2,6 +2,7 @@ package getsvc
import (
"context"
"crypto/ecdsa"
"errors"
"fmt"
"io"
@ -16,7 +17,6 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/network"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/internal"
internalclient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/internal/client"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/util"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
)
@ -24,16 +24,13 @@ type getRequestForwarder struct {
OnceResign *sync.Once
OnceHeaderSending *sync.Once
GlobalProgress int
KeyStorage *util.KeyStorage
Key *ecdsa.PrivateKey
Request *objectV2.GetRequest
Stream *streamObjectWriter
}
func (f *getRequestForwarder) forwardRequestToNode(ctx context.Context, addr network.Address, c client.MultiAddressClient, pubkey []byte) (*object.Object, error) {
carpawell commented 2023-04-03 15:50:40 +00:00
Outdated
Review
Copy link

so not used second return value?

so not used second return value?
dstepanov-yadro commented 2023-04-03 16:54:37 +00:00
Outdated
Review
Copy link

forwardRequestToNode is used as an argument for func groupAddressRequestForwarder(...) function. groupAddressRequestForwarder requires two values to be returned. It is possible to use a lambda function, but I don't like it.

Do you think that with a lambda and one return value it will be easier to understand?

```forwardRequestToNode``` is used as an argument for ```func groupAddressRequestForwarder(...)``` function. ```groupAddressRequestForwarder``` requires two values to be returned. It is possible to use a lambda function, but I don't like it. Do you think that with a lambda and one return value it will be easier to understand?
carpawell commented 2023-04-05 07:33:56 +00:00
Outdated
Review
Copy link

Do you think that with a lambda and one return value it will be easier to understand?

no, do not like lambdas usually as more complex thing for a reader.

is used as an argument for func groupAddressRequestForwarder(...) function

originally, comment was about that: seems like anything that is ...forwarder... should not return objects. if groupAddressRequestForwarder is used somewhere else AND that is not a complete refactor and we plan to reorganize it later, im ok with that

> Do you think that with a lambda and one return value it will be easier to understand? no, do not like lambdas usually as more complex thing for a reader. > is used as an argument for func groupAddressRequestForwarder(...) function originally, comment was about that: seems like anything that is `...forwarder...` should not return objects. if `groupAddressRequestForwarder` is used somewhere else AND that is not a complete refactor and we plan to reorganize it later, im ok with that
dstepanov-yadro commented 2023-04-05 08:11:11 +00:00
Outdated
Review
Copy link

HEAD request requires return result from forwarder

HEAD request requires return result from forwarder
key, err := f.KeyStorage.GetKey(nil)
if err != nil {
return nil, err
}
var err error
// once compose and resign forwarding request
f.OnceResign.Do(func() {
@ -44,7 +41,7 @@ func (f *getRequestForwarder) forwardRequestToNode(ctx context.Context, addr net
metaHdr.SetOrigin(f.Request.GetMetaHeader())
writeCurrentVersion(metaHdr)
f.Request.SetMetaHeader(metaHdr)
err = signature.SignServiceMessage(key, f.Request)
err = signature.SignServiceMessage(f.Key, f.Request)
})
if err != nil {

11
pkg/services/object/get/v2/get_range_forwarder.go
View file

@ -2,6 +2,7 @@ package getsvc
import (
"context"
"crypto/ecdsa"
"errors"
"fmt"
"io"
@ -16,23 +17,19 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/network"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/internal"
internalclient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/internal/client"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/util"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
)
type getRangeRequestForwarder struct {
OnceResign *sync.Once
GlobalProgress int
KeyStorage *util.KeyStorage
Key *ecdsa.PrivateKey
Request *objectV2.GetRangeRequest
Stream *streamObjectRangeWriter
}
func (f *getRangeRequestForwarder) forwardRequestToNode(ctx context.Context, addr network.Address, c client.MultiAddressClient, pubkey []byte) (*object.Object, error) {
key, err := f.KeyStorage.GetKey(nil)
if err != nil {
return nil, err
}
var err error
// once compose and resign forwarding request
f.OnceResign.Do(func() {
@ -45,7 +42,7 @@ func (f *getRangeRequestForwarder) forwardRequestToNode(ctx context.Context, add
f.Request.SetMetaHeader(metaHdr)
err = signature.SignServiceMessage(key, f.Request)
err = signature.SignServiceMessage(f.Key, f.Request)
})
if err != nil {

11
pkg/services/object/get/v2/head_forwarder.go
View file

@ -2,6 +2,7 @@ package getsvc
import (
"context"
"crypto/ecdsa"
"errors"
"fmt"
"sync"
@ -15,7 +16,6 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/client"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/network"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/internal"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/util"
frostfscrypto "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/crypto"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id"
@ -26,17 +26,12 @@ type headRequestForwarder struct {
Response *objectV2.HeadResponse
OnceResign *sync.Once
ObjectAddr oid.Address
KeyStorage *util.KeyStorage
Key *ecdsa.PrivateKey
}
func (f *headRequestForwarder) forwardRequestToNode(ctx context.Context, addr network.Address, c client.MultiAddressClient, pubkey []byte) (*object.Object, error) {
var err error
key, err := f.KeyStorage.GetKey(nil)
if err != nil {
return nil, err
}
// once compose and resign forwarding request
f.OnceResign.Do(func() {
// compose meta header of the local server
@ -48,7 +43,7 @@ func (f *headRequestForwarder) forwardRequestToNode(ctx context.Context, addr ne
f.Request.SetMetaHeader(metaHdr)
err = signature.SignServiceMessage(key, f.Request)
err = signature.SignServiceMessage(f.Key, f.Request)
})
if err != nil {

21
pkg/services/object/get/v2/util.go
View file

@ -56,11 +56,16 @@ func (s *Service) toPrm(req *objectV2.GetRequest, stream objectSvc.GetObjectStre
p.SetObjectWriter(streamWrapper)
if !commonPrm.LocalOnly() {
key, err := s.keyStorage.GetKey(nil)
if err != nil {
fyrchik commented 2023-04-03 08:37:44 +00:00
Outdated
Review
Copy link

Is there any reason you don't just use non-pointer field in struct and omit the initialization?

Is there any reason you don't just use non-pointer field in struct and omit the initialization?
dstepanov-yadro commented 2023-04-03 09:56:28 +00:00
Outdated
Review
Copy link

This is the easiest way to meet this requirement: Values containing the types defined in this package should not be copied. (https://pkg.go.dev/sync)

This is the easiest way to meet this requirement: *Values containing the types defined in this package should not be copied.* (https://pkg.go.dev/sync)
carpawell commented 2023-04-03 15:57:26 +00:00
Outdated
Review
Copy link

but forwarder is already a pointer? i am ok with that, just want to come to some approach that will be commonly adopted in that repo (the other code do not use pointer inside a pointer i guess)

but `forwarder` is already a pointer? i am ok with that, just want to come to some approach that will be commonly adopted in that repo (the other code do not use pointer inside a pointer i guess)
dstepanov-yadro commented 2023-04-03 17:22:37 +00:00
Outdated
Review
Copy link

the other code do not use pointer inside a pointer i guess

But you should always think how holder of sync.Mutex (RWMutex, Once...) will be used.

> the other code do not use pointer inside a pointer i guess But you should always think how holder of ```sync.Mutex (RWMutex, Once...)``` will be used.
return nil, err
}
forwarder := &getRequestForwarder{
OnceResign: &sync.Once{},
OnceHeaderSending: &sync.Once{},
GlobalProgress: 0,
KeyStorage: s.keyStorage,
Key: key,
Request: req,
Stream: streamWrapper,
}
@ -107,10 +112,15 @@ func (s *Service) toRangePrm(req *objectV2.GetRangeRequest, stream objectSvc.Get
}
if !commonPrm.LocalOnly() {
key, err := s.keyStorage.GetKey(nil)
if err != nil {
return nil, err
}
forwarder := &getRangeRequestForwarder{
OnceResign: &sync.Once{},
GlobalProgress: 0,
KeyStorage: s.keyStorage,
Key: key,
Request: req,
Stream: streamWrapper,
}
@ -239,12 +249,17 @@ func (s *Service) toHeadPrm(ctx context.Context, req *objectV2.HeadRequest, resp
return p, nil
}
key, err := s.keyStorage.GetKey(nil)
if err != nil {
return nil, err
}
forwarder := &headRequestForwarder{
Request: req,
Response: resp,
OnceResign: &sync.Once{},
ObjectAddr: objAddr,
KeyStorage: s.keyStorage,
Key: key,
}
p.SetRequestForwarder(groupAddressRequestForwarder(forwarder.forwardRequestToNode))