[#142] Fix multipart-objects download #143
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-http-gw#143
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-http-gw:fix_multipart"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com
d14cb1f695
to33dba5d68c
33dba5d68c
toea38b4e6c8
@ -35,4 +35,2 @@
}
// PrmContainer groups parameters of FrostFS.Container operation.
type PrmContainer struct {
Why do we move these parameters?
@ -48,0 +125,4 @@
return nil, fmt.Errorf("unmarshal combined object parts: %w", err)
}
isEncrypted := FormEncryptionInfo(p.objInfo.Headers).Enabled
Do we really need this? I mean will we support decrypting and/or range? If no then we can simplify reading logic a lot
Consider we don't have any decryption in GetObject, we can omit encryption in multiparts too
ea38b4e6c8
to1cf28fca80
1cf28fca80
to02cf52dd57
@ -64,1 +64,3 @@
Address oid.Address
Address oid.Address
Container cid.ID
Object oid.ID
Why do we need
Container
andObject
? We've already hadAddress
@ -77,0 +79,4 @@
Container cid.ID
Object oid.ID
The same as previous comment
02cf52dd57
to0b03f693a2
@ -122,0 +123,4 @@
// payload range
Off, Ln uint64
ObjInfo *data.ObjectInfo
Why do we need
ObjectInfo
It seems we can keep only object address.Also
BktInfo
is unnecessary@ -122,0 +125,4 @@
ObjInfo *data.ObjectInfo
BktInfo *data.BucketInfo
Log *zap.Logger
Why do we need
Log
?@ -9,2 +12,4 @@
)
const (
frostFSSystemMetadataPrefix = "S3-"
Do we really need separate const for that?
@ -80,3 +102,2 @@
}
req.Response.Header.Set(fasthttp.HeaderContentLength, strconv.FormatUint(payloadSize, 10))
var contentType string
for _, attr := range rObj.Header.Attributes() {
Is it necessary to make such changes?
Can we make diff as small as possible?
@ -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)
It seems we have to add
return
statement in line belowOr drop this line if we don't fail request if timestamp is invalid
@ -0,0 +17,4 @@
// payload range
Off, Ln uint64
Oid oid.ID
The field should name
OID
@ -0,0 +18,4 @@
Off, Ln uint64
Oid oid.ID
BktInfo *data.BucketInfo
Do we really need
BucketInfo
? It seems that having CID be enough@ -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
This field is unused
0b03f693a2
to16548f1a30
New commits pushed, approval review dismissed automatically according to repository settings
@ -51,0 +51,4 @@
type getMultiobjectBodyParams struct {
obj *Object
req request
bktinfo *data.BucketInfo
Why do we need bktInfo?
@ -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)) {
Why do we need
*data.BucketInfo
?@ -48,0 +67,4 @@
ctx := p.req.RequestCtx
params := PrmInitMultiObjectReader{
Off: 0,
Ln: 0,
We can omit setting fields with default values
@ -81,1 +83,3 @@
var contentType string
req.setIDs(rObj.Header)
payload = rObj.Payload
payloadSize = rObj.Header.PayloadSize()
Please, use:
@ -107,3 +106,4 @@
contentType = val
case object.AttributeFilePath:
filepath = val
if filename == "" {
This if should be placed after
for
loop. We have not guarantees that attributeFileName
occurs beforeFilePath
inrObj.Header.Attributes()
@ -0,0 +36,4 @@
}
type readerInitiator interface {
InitFrostFSObjectPayloadReader(ctx context.Context, p GetFrostFSParams) (io.Reader, error)
Why do we use
io.Reader
rather thanio.ReadCloser
?I undesrtand that this is copy-paste from s3-gw. But probably in s3-gw we also should fix it
cc @alexvanin
16548f1a30
toe844ace43d
e844ace43d
to7b91e998ea
@ -0,0 +174,4 @@
n, err = x.curReader.Read(p)
if err != nil {
if err := x.curReader.Close(); err != nil {
return n, err
I suppose it's better to keep original error something like this (or just log closing error)
7b91e998ea
to495f745535
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings