Remove re-slicing for multipart #931

smallhive · 2024-02-22T12:03:20Z

Closes #843

codecov · 2024-02-22T12:05:48Z

Codecov Report

Attention: Patch coverage is 1.91388% with 615 lines in your changes are missing coverage. Please review.

Project coverage is 26.92%. Comparing base (aadef93) to head (127dd3e).
Report is 1 commits behind head on master.

Files	Patch %	Lines
api/layer/multipart_upload.go	0.00%	265 Missing ⚠️
api/layer/neofs_mock.go	8.54%	102 Missing and 5 partials ⚠️
internal/neofs/tree.go	0.00%	86 Missing ⚠️
api/layer/tree_mock.go	0.00%	55 Missing ⚠️
api/layer/object.go	0.00%	49 Missing and 1 partial ⚠️
internal/neofs/neofs.go	0.00%	47 Missing ⚠️
api/layer/layer.go	33.33%	3 Missing and 1 partial ⚠️
api/handler/acl.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
- Coverage   27.97%   26.92%   -1.06%     
==========================================
  Files          87       87              
  Lines       13590    14129     +539     
==========================================
+ Hits         3802     3804       +2     
- Misses       9371     9903     +532     
- Partials      417      422       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

smallhive · 2024-02-29T10:58:00Z

Rebased

carpawell

Can the internal slicer option ever be turned off?

internal/neofs/tree.go

api/layer/multipart_upload.go

carpawell · 2024-03-04T17:19:48Z

api/layer/multipart_upload.go

+		id           oid.ID
+		elements     []string
+		creationTime = TimeNow(ctx)
+		// User may upload part large maxObjectSize in NeoFS. From users point of view it is a single object.


not sure i can understand this comment properly. I've read something about ETag (the only thing this var is used for) but cannot correlate it with this comment. can you please extend it or tell me what a reader should understand here?

Parts can be reuploaded and they're identified by etags.

sure, just not get how this comments helps and what it wants to say to me

carpawell · 2024-03-04T17:21:26Z

api/layer/layer.go

@@ -266,13 +268,20 @@ func (f MsgHandlerFunc) HandleMessage(ctx context.Context, msg *nats.Msg) error
 // NewLayer creates an instance of a layer. It checks credentials
 // and establishes gRPC connection with the node.
 func NewLayer(log *zap.Logger, neoFS NeoFS, config *Config) Client {
+	buffers := sync.Pool{}
+	buffers.New = func() any {
+		b := make([]byte, neoFS.MaxObjectSize())


IMO, a potential OOM killer is here, the same way we already fixed this in SDK

Links to changes, please

Using sync.Pool showed better memory using profile after load tests. It Already works here

neofs-s3-gw/internal/neofs/neofs.go

Line 331 in 4717bd8

data := x.buffers.Get()

Links to changes, please

nspcc-dev/neofs-sdk-go#539

Using sync.Pool showed better memory using profile after load tests. It Already works here

yes, but putting 1K objects 1-byte sized simultaneously leads to 64GB memory usage but requires only 1KB in fact. we already know we may be quite successive at running out of 1TB memory, not a cool story

but ok, as i understand we will return to this later

api/layer/multipart_upload.go

internal/neofs/neofs.go

cmd/s3-gw/app_settings.go

roman-khimov

What happens if we're to reupload a part?

api/layer/multipart_upload.go

roman-khimov · 2024-03-05T19:36:35Z

api/layer/multipart_upload.go

+		id           oid.ID
+		elements     []string
+		creationTime = TimeNow(ctx)
+		// User may upload part large maxObjectSize in NeoFS. From users point of view it is a single object.


Parts can be reuploaded and they're identified by etags.

api/layer/multipart_upload.go

roman-khimov · 2024-03-05T19:41:43Z

api/layer/multipart_upload.go

+	}
+
+	dt := n.buffers.Get()
+	chunk := dt.(*[]byte)


Can we leverage p.Size here for buffer allocation?

Do you mean just in place buffer like b := make([]byte, p.size)? Buffers already works here

neofs-s3-gw/internal/neofs/neofs.go

Line 331 in 4717bd8

data := x.buffers.Get()

and after many tests this approach shows better memory using, rather in place buffer

roman-khimov · 2024-03-05T19:44:08Z

api/layer/multipart_upload.go

+	prm := PrmObjectCreate{
+		Container:       bktInfo.CID,
+		Creator:         bktInfo.Owner,
+		Attributes:      attributes,


Should we have an attribute identifying particular part?

I don't see the situation when we need it. Do you have a case?

It's just a suggestion, if it can't simplify anything now then OK.

roman-khimov · 2024-03-05T19:45:37Z

api/layer/multipart_upload.go

+			if len(lastPart.MultipartHash) > 0 {
+				splitPreviousID = lastPart.OID
+
+				if binaryUnmarshaler, ok := multipartHash.(encoding.BinaryUnmarshaler); ok && len(lastPart.MultipartHash) > 0 {


Can be moved to some helper, duplicating.

api/layer/neofs_mock.go

cmd/s3-gw/app_settings.go

api/data/tree.go

api/layer/neofs.go

api/layer/multipart_upload.go

internal/neofs/neofs.go

internal/neofs/tree.go

internal/neofs/neofs.go

cthulhu-rider · 2024-03-07T07:41:12Z

main commit with reslicing removal contains only issue ref which is good for GH but not enough for git history overall. Lets write some text about motivation and used approach

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

smallhive · 2024-03-15T05:15:59Z

What happens if we're to reupload a part?

Everything will be broken. We collect full object hash moving from part to part. If you re-upload any part in the middle, we will have to re-read all parts after it, recalculate hashes, and re-upload parts

roman-khimov · 2024-03-15T08:51:24Z

Everything will be broken. We collect full object hash moving from part to part. If you re-upload any part in the middle, we will have to re-read all parts after it, recalculate hashes, and re-upload parts

Cool. We need to do this. And that's where per-part IDs can become handy because

Part numbers can be any number from 1 to 10,000, inclusive. A part number uniquely identifies a part
and also defines its position within the object being created. If you upload a new part using the same
part number that was used with a previous part, the previously uploaded part is overwritten.

And btw,

Each part must be at least 5 MB in size, except the last part.

smallhive · 2024-03-19T06:40:27Z

Everything will be broken. We collect full object hash moving from part to part. If you re-upload any part in the middle, we will have to re-read all parts after it, recalculate hashes, and re-upload parts

Cool. We need to do this. And that's where per-part IDs can become handy because

Part numbers can be any number from 1 to 10,000, inclusive. A part number uniquely identifies a part
and also defines its position within the object being created. If you upload a new part using the same
part number that was used with a previous part, the previously uploaded part is overwritten.

And btw,

Each part must be at least 5 MB in size, except the last part.

Added logic for re-upload parts

Closes #843. Before, multipartComplete read all parts of Big object to the memory, combine them and generate final Big object. These step consume time and memory, eventually any system will fail to load all parts in mem or timeout during the process. After, object slicing process works from the first uploaded part. Calculating each part hash and whole object hash during whole process. Storing object hash state to each part metadata in tree service. Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

It is required, because we have to re-evaluate object payload hash. Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

roman-khimov

Buffering is the only weakness left here, but we can try optimizing it in future, for now I'd like to see functional requirements met.

api/layer/neofs_mock.go

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch 2 times, most recently from 5814442 to 431c312 Compare February 28, 2024 11:32

smallhive marked this pull request as ready for review February 28, 2024 11:32

smallhive requested a review from roman-khimov February 28, 2024 11:32

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch from 431c312 to 68451d2 Compare February 28, 2024 11:34

roman-khimov requested review from carpawell and cthulhu-rider February 29, 2024 06:56

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch from 68451d2 to e0d1565 Compare February 29, 2024 10:57

carpawell reviewed Mar 4, 2024

View reviewed changes

roman-khimov reviewed Mar 5, 2024

View reviewed changes

cthulhu-rider reviewed Mar 7, 2024

View reviewed changes

smallhive added 3 commits March 14, 2024 13:33

layer: Remove stderrors package alias

e7d03c3

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

api: Store SplitID in multipart meta info

a079f9c

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

go.mod: Update github.com/nspcc-dev/tzhash to the v1.8.0

ef9a5ba

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch 3 times, most recently from 99144d1 to 55ab89a Compare March 15, 2024 05:03

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch 2 times, most recently from c7a2e4e to 4188fe1 Compare March 15, 2024 07:29

smallhive requested review from roman-khimov, cthulhu-rider and carpawell March 15, 2024 07:30

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch from 477e53a to 0d215e4 Compare March 19, 2024 06:41

smallhive added 3 commits March 19, 2024 10:46

layer: Remove unused reader

a748b72

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

layer: Actualize multipart tests

2548e50

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

layer: Re-upload all parts after current one

127dd3e

It is required, because we have to re-evaluate object payload hash. Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

smallhive force-pushed the 843-complete-multipart-upload-fails-with-timeout branch from 0d215e4 to 127dd3e Compare March 19, 2024 06:47

roman-khimov approved these changes Mar 24, 2024

View reviewed changes

cthulhu-rider approved these changes Mar 25, 2024

View reviewed changes

carpawell reviewed Mar 25, 2024

View reviewed changes

api/layer/neofs_mock.go Show resolved Hide resolved

This was referenced Mar 25, 2024

Switch to v2 object split scheme #937

Closed

Excessive memory consumption in slicing code #938

Closed

roman-khimov merged commit 20f55c4 into master Mar 26, 2024
11 of 15 checks passed

roman-khimov deleted the 843-complete-multipart-upload-fails-with-timeout branch March 26, 2024 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove re-slicing for multipart #931

Remove re-slicing for multipart #931

smallhive commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

smallhive commented Feb 29, 2024

carpawell left a comment

carpawell Mar 4, 2024

roman-khimov Mar 5, 2024

carpawell Mar 25, 2024

carpawell Mar 4, 2024

smallhive Mar 14, 2024

smallhive Mar 15, 2024

carpawell Mar 25, 2024

roman-khimov left a comment

roman-khimov Mar 5, 2024

roman-khimov Mar 5, 2024

smallhive Mar 15, 2024

roman-khimov Mar 5, 2024

smallhive Mar 15, 2024

roman-khimov Mar 15, 2024

roman-khimov Mar 5, 2024

cthulhu-rider commented Mar 7, 2024 •

edited

Loading

smallhive commented Mar 15, 2024

roman-khimov commented Mar 15, 2024

smallhive commented Mar 19, 2024

roman-khimov left a comment

Remove re-slicing for multipart #931

Remove re-slicing for multipart #931

Conversation

smallhive commented Feb 22, 2024 • edited Loading

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

smallhive commented Feb 29, 2024

carpawell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider commented Mar 7, 2024 • edited Loading

smallhive commented Mar 15, 2024

roman-khimov commented Mar 15, 2024

smallhive commented Mar 19, 2024

roman-khimov left a comment

Choose a reason for hiding this comment

smallhive commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

cthulhu-rider commented Mar 7, 2024 •

edited

Loading