[#142] Fix multipart-objects download #143

Merged
alexvanin merged 1 commit from nzinkevich/frostfs-http-gw:fix_multipart into master 2024-10-17 11:00:23 +00:00
Member

Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich added 1 commit 2024-09-19 05:40:44 +00:00
[#142] Fix multipart-objects download
All checks were successful
/ DCO (pull_request) Successful in 37s
/ Vulncheck (pull_request) Successful in 1m1s
/ Builds (pull_request) Successful in 1m33s
/ Lint (pull_request) Successful in 3m13s
/ Tests (pull_request) Successful in 1m15s
d14cb1f695
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich requested review from storage-services-committers 2024-09-19 06:02:09 +00:00
nzinkevich requested review from storage-services-developers 2024-09-19 06:02:09 +00:00
nzinkevich force-pushed fix_multipart from d14cb1f695 to 33dba5d68c 2024-09-19 10:58:50 +00:00 Compare
nzinkevich force-pushed fix_multipart from 33dba5d68c to ea38b4e6c8 2024-09-19 13:07:35 +00:00 Compare
dkirillov reviewed 2024-09-24 08:45:02 +00:00
@ -35,4 +35,2 @@
}
// PrmContainer groups parameters of FrostFS.Container operation.
type PrmContainer struct {
Member

Why do we move these parameters?

Why do we move these parameters?
nzinkevich marked this conversation as resolved
@ -48,0 +125,4 @@
return nil, fmt.Errorf("unmarshal combined object parts: %w", err)
}
isEncrypted := FormEncryptionInfo(p.objInfo.Headers).Enabled
Member

Do we really need this? I mean will we support decrypting and/or range? If no then we can simplify reading logic a lot

Do we really need this? I mean will we support decrypting and/or range? If no then we can simplify reading logic a lot
Author
Member

Consider we don't have any decryption in GetObject, we can omit encryption in multiparts too

Consider we don't have any decryption in GetObject, we can omit encryption in multiparts too
nzinkevich marked this conversation as resolved
nzinkevich force-pushed fix_multipart from ea38b4e6c8 to 1cf28fca80 2024-10-03 09:31:20 +00:00 Compare
nzinkevich force-pushed fix_multipart from 1cf28fca80 to 02cf52dd57 2024-10-03 09:41:34 +00:00 Compare
dkirillov reviewed 2024-10-07 07:55:37 +00:00
@ -64,1 +64,3 @@
Address oid.Address
Address oid.Address
Container cid.ID
Object oid.ID
Member

Why do we need Container and Object? We've already had Address

Why do we need `Container` and `Object`? We've already had `Address`
nzinkevich marked this conversation as resolved
dkirillov reviewed 2024-10-07 07:55:58 +00:00
@ -77,0 +79,4 @@
Container cid.ID
Object oid.ID
Member

The same as previous comment

The same as previous comment
nzinkevich marked this conversation as resolved
nzinkevich force-pushed fix_multipart from 02cf52dd57 to 0b03f693a2 2024-10-07 11:40:42 +00:00 Compare
alexvanin approved these changes 2024-10-14 11:38:58 +00:00
Dismissed
dkirillov reviewed 2024-10-15 06:34:00 +00:00
@ -122,0 +123,4 @@
// payload range
Off, Ln uint64
ObjInfo *data.ObjectInfo
Member

Why do we need ObjectInfo It seems we can keep only object address.
Also BktInfo is unnecessary

Why do we need `ObjectInfo` It seems we can keep only object address. Also `BktInfo` is unnecessary
dkirillov marked this conversation as resolved
@ -122,0 +125,4 @@
ObjInfo *data.ObjectInfo
BktInfo *data.BucketInfo
Log *zap.Logger
Member

Why do we need Log?

Why do we need `Log`?
dkirillov marked this conversation as resolved
@ -9,2 +12,4 @@
)
const (
frostFSSystemMetadataPrefix = "S3-"
Member

Do we really need separate const for that?

Do we really need separate const for that?
dkirillov marked this conversation as resolved
@ -80,3 +102,2 @@
}
req.Response.Header.Set(fasthttp.HeaderContentLength, strconv.FormatUint(payloadSize, 10))
var contentType string
for _, attr := range rObj.Header.Attributes() {
Member

Is it necessary to make such changes?
Can we make diff as small as possible?

Is it necessary to make such changes? Can we make diff as small as possible?
dkirillov marked this conversation as resolved
@ -76,0 +85,4 @@
req.log.Error(logs.CouldntParseCreationDate,
zap.String("val", attrs[object.AttributeTimestamp]),
zap.Error(err))
response.Error(req.RequestCtx, "failed to convert timestamp: "+err.Error(), fasthttp.StatusInternalServerError)
Member

It seems we have to add return statement in line below

It seems we have to add `return` statement in line below
Member

Or drop this line if we don't fail request if timestamp is invalid

Or drop this line if we don't fail request if timestamp is invalid
dkirillov marked this conversation as resolved
@ -0,0 +17,4 @@
// payload range
Off, Ln uint64
Oid oid.ID
Member

The field should name OID

The field should name `OID`
dkirillov marked this conversation as resolved
@ -0,0 +18,4 @@
Off, Ln uint64
Oid oid.ID
BktInfo *data.BucketInfo
Member

Do we really need BucketInfo? It seems that having CID be enough

Do we really need `BucketInfo`? It seems that having CID be enough
dkirillov marked this conversation as resolved
@ -0,0 +33,4 @@
// MultiObjectReader implements io.Reader of payloads of the object list stored in the FrostFS network.
type MultiObjectReader struct {
ctx context.Context
log *zap.Logger
Member

This field is unused

This field is unused
dkirillov marked this conversation as resolved
nzinkevich force-pushed fix_multipart from 0b03f693a2 to 16548f1a30 2024-10-15 10:05:05 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-10-15 10:05:06 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dkirillov reviewed 2024-10-15 11:13:34 +00:00
@ -51,0 +51,4 @@
type getMultiobjectBodyParams struct {
obj *Object
req request
bktinfo *data.BucketInfo
Member

Why do we need bktInfo?

Why do we need bktInfo?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-15 11:15:34 +00:00
@ -180,3 +190,3 @@
// byAddress is a wrapper for function (e.g. request.headObject, request.receiveFile) that
// prepares request and object address to it.
func (h *Handler) byAddress(c *fasthttp.RequestCtx, f func(context.Context, request, oid.Address)) {
func (h *Handler) byAddress(c *fasthttp.RequestCtx, f func(context.Context, request, oid.Address, *data.BucketInfo)) {
Member

Why do we need *data.BucketInfo?

Why do we need `*data.BucketInfo`?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-15 11:16:42 +00:00
@ -48,0 +67,4 @@
ctx := p.req.RequestCtx
params := PrmInitMultiObjectReader{
Off: 0,
Ln: 0,
Member

We can omit setting fields with default values

We can omit setting fields with default values
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-15 11:19:02 +00:00
@ -81,1 +83,3 @@
var contentType string
req.setIDs(rObj.Header)
payload = rObj.Payload
payloadSize = rObj.Header.PayloadSize()
Member

Please, use:

payload := rObj.Payload
payloadSize := rObj.Header.PayloadSize()
Please, use: ```golang payload := rObj.Payload payloadSize := rObj.Header.PayloadSize() ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-15 11:21:36 +00:00
@ -107,3 +106,4 @@
contentType = val
case object.AttributeFilePath:
filepath = val
if filename == "" {
Member

This if should be placed after for loop. We have not guarantees that attribute FileName occurs before FilePath in rObj.Header.Attributes()

This if should be placed after `for` loop. We have not guarantees that attribute `FileName` occurs before `FilePath` in `rObj.Header.Attributes()`
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-15 11:30:57 +00:00
@ -0,0 +36,4 @@
}
type readerInitiator interface {
InitFrostFSObjectPayloadReader(ctx context.Context, p GetFrostFSParams) (io.Reader, error)
Member

Why do we use io.Reader rather than io.ReadCloser?
I undesrtand that this is copy-paste from s3-gw. But probably in s3-gw we also should fix it

cc @alexvanin

Why do we use `io.Reader` rather than `io.ReadCloser`? I undesrtand that this is copy-paste from s3-gw. But probably in s3-gw we also should fix it cc @alexvanin
dkirillov marked this conversation as resolved
nzinkevich force-pushed fix_multipart from 16548f1a30 to e844ace43d 2024-10-15 12:45:36 +00:00 Compare
nzinkevich force-pushed fix_multipart from e844ace43d to 7b91e998ea 2024-10-15 12:52:54 +00:00 Compare
alexvanin approved these changes 2024-10-15 13:04:25 +00:00
Dismissed
dkirillov reviewed 2024-10-15 14:09:25 +00:00
@ -0,0 +174,4 @@
n, err = x.curReader.Read(p)
if err != nil {
if err := x.curReader.Close(); err != nil {
return n, err
Member

I suppose it's better to keep original error something like this (or just log closing error)

if closeErr := x.curReader.Close(); closeErr != nil {
		return n, fmt.Errorf("%w (close err: %v)", err, closeErr)
}
I suppose it's better to keep original error something like this (or just log closing error) ```golang if closeErr := x.curReader.Close(); closeErr != nil { return n, fmt.Errorf("%w (close err: %v)", err, closeErr) } ```
dkirillov marked this conversation as resolved
dkirillov approved these changes 2024-10-15 14:09:34 +00:00
Dismissed
pogpp approved these changes 2024-10-15 14:12:20 +00:00
Dismissed
nzinkevich force-pushed fix_multipart from 7b91e998ea to 495f745535 2024-10-15 14:20:13 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-10-15 14:20:14 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

nzinkevich dismissed dkirillov's review 2024-10-15 14:20:14 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

nzinkevich dismissed pogpp's review 2024-10-15 14:20:14 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dkirillov approved these changes 2024-10-15 14:30:44 +00:00
nzinkevich was assigned by dkirillov 2024-10-15 14:31:02 +00:00
dkirillov requested review from storage-services-developers 2024-10-15 14:31:08 +00:00
dkirillov requested review from storage-services-committers 2024-10-15 14:31:09 +00:00
mbiryukova approved these changes 2024-10-16 12:29:02 +00:00
alexvanin approved these changes 2024-10-17 11:00:18 +00:00
alexvanin merged commit 495f745535 into master 2024-10-17 11:00:23 +00:00
alexvanin deleted branch fix_multipart 2024-10-17 11:00:24 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 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-http-gw#143
No description provided.