Upgrade sdk to rc10 #261

smallhive · 2023-08-08T10:48:00Z

closes #260

codecov · 2023-08-08T10:55:50Z

Codecov Report

Merging #261 (465257d) into master (e6e7941) will decrease coverage by 0.33%.
Report is 3 commits behind head on master.
The diff coverage is 24.29%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   34.62%   34.29%   -0.33%     
==========================================
  Files          10       10              
  Lines        1369     1382      +13     
==========================================
  Hits          474      474              
- Misses        852      866      +14     
+ Partials       43       42       -1

Files Changed	Coverage Δ
downloader/download.go	`8.15% <0.00%> (-0.16%)`	⬇️
downloader/head.go	`0.00% <0.00%> (ø)`
settings.go	`52.24% <ø> (-1.31%)`	⬇️
uploader/upload.go	`0.00% <0.00%> (ø)`
app.go	`60.74% <74.28%> (+2.76%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

resolver/resolver.go

app.go

downloader/download.go

downloader/head.go

uploader/upload.go

Signed-off-by: Evgenii Baidakov <[email protected]>

cthulhu-rider

overall LGTM, some cosmetics may be applied

cthulhu-rider · 2023-08-10T07:56:49Z

utils/util.go

-func GetContainerID(ctx context.Context, containerID string, resolver *resolver.ContainerResolver) (*cid.ID, error) {
-	cnrID := new(cid.ID)
+func GetContainerID(ctx context.Context, containerID string, resolver resolver.Resolver) (*cid.ID, error) {
+	cnrID := cid.ID{}


seems like useless change

This change allows not to change other lines in the function

ok, missed that Resolve changed signature. I'd prefer

Suggested change

cnrID := cid.ID{}

var cnrID cid.ID

but dont mind

app.go

downloader/download.go

cthulhu-rider · 2023-08-10T08:27:22Z

uploader/upload.go

 		return
 	}

+	for {


can io.CopyBuffer be used here? i see that we pass different messages into response.Error, but taking into account we always respond with 500 don't think it's worth code complication

Sure, but as how benchmark showed in Slicer, this isn't so effective and consumes more memory

then maybe add a comment about this before for so that this question does not arise in the future?

Sounds good, but maybe I should make some local benchmark tests for object upload, separate from the slicer. Maybe it was a special case in than code.
Give me some time, I will post the result of my research here

I'd expect it to be around the same with io.Copy, but pay attention to buffer size (#148).

Why? The object may be big. We need to read it part by part. Writing yes, right now a little bit shorter, but anyway

Why not just io.Copy()? This would do the trick without loops/maxsizes. It can also be optimized if we're to provide ReaderFrom (in the SDK) and/or WriterTo (in the multipart handler).

I consider we want to use bigger buffers. io.Copy uses 32kb buffers (if corresponding reader is not a LimitedReader, in our case in isn't), which too small, according to task #148.

But I agree, io.CopyBuffer will help to remove the for

Updated to io.CopyBuffer

smallhive · 2023-08-11T07:12:35Z

Updated objectPut with io package, make maxObjectSize configurable for Uploader

cthulhu-rider · 2023-08-11T07:19:19Z

app.go

 	"github.com/nspcc-dev/neofs-sdk-go/user"
 	"github.com/spf13/viper"
 	"github.com/valyala/fasthttp"
 	"go.uber.org/zap"
 )

+const (
+	defaultObjectSize = int64(2 << 21) // 2MB


that's not 2MB but 4. If u need 2MB, then 1 << 21 or 2 << 20 (like more). If 4, then twice

Sure, thought about 2^21, and as a result 2 << 21. Updated to 1 << 21

cthulhu-rider · 2023-08-11T07:23:53Z

uploader/upload.go

+			continue
+		}
+
+		if !errors.Is(err, io.EOF) {


CopyN doesn't return EOF

Suggested change

if !errors.Is(err, io.EOF) {

if err != nil {

Returns and without this check, it doesn't work

app.go

roman-khimov · 2023-08-11T08:55:22Z

uploader/upload.go

 		return
 	}

+	for {


Why not just io.Copy()? This would do the trick without loops/maxsizes. It can also be optimized if we're to provide ReaderFrom (in the SDK) and/or WriterTo (in the multipart handler).

Signed-off-by: Evgenii Baidakov <[email protected]>

closes #260 Signed-off-by: Evgenii Baidakov <[email protected]>

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive marked this pull request as ready for review August 8, 2023 10:57

smallhive requested a review from roman-khimov August 8, 2023 10:57

roman-khimov reviewed Aug 9, 2023

View reviewed changes

resolver/resolver.go Show resolved Hide resolved

app.go Show resolved Hide resolved

downloader/download.go Outdated Show resolved Hide resolved

downloader/head.go Show resolved Hide resolved

uploader/upload.go Outdated Show resolved Hide resolved

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch 3 times, most recently from 15f0295 to 7a79448 Compare August 10, 2023 06:37

smallhive requested review from roman-khimov and cthulhu-rider August 10, 2023 06:42

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch from 7a79448 to a0b66e0 Compare August 10, 2023 07:33

tests: Update tests to use the latest aio container

2afeb4e

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch from a0b66e0 to 0f9eb91 Compare August 10, 2023 07:44

cthulhu-rider previously approved these changes Aug 10, 2023

View reviewed changes

smallhive dismissed cthulhu-rider’s stale review via c3a684d August 10, 2023 08:47

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch 4 times, most recently from 99d25e0 to bb91d85 Compare August 11, 2023 07:11

smallhive requested a review from cthulhu-rider August 11, 2023 07:12

cthulhu-rider reviewed Aug 11, 2023

View reviewed changes

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch from bb91d85 to cd7049b Compare August 11, 2023 07:25

roman-khimov reviewed Aug 11, 2023

View reviewed changes

smallhive added 2 commits August 11, 2023 13:08

resolver: Replace resolver with NNS contract bindings

de9e33f

Signed-off-by: Evgenii Baidakov <[email protected]>

*: Update SDK to the latest version

e8b2cd3

closes #260 Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch from cd7049b to 50b36c6 Compare August 11, 2023 09:21

smallhive added 3 commits August 11, 2023 13:53

*: Remove pool's deprecated methods

4f93678

Signed-off-by: Evgenii Baidakov <[email protected]>

uploader: Make maxObjectSize configurable

e7d79fc

Signed-off-by: Evgenii Baidakov <[email protected]>

uploader: Update errors check

465257d

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the 260-upgrade-sdk-to-rc10 branch from 50b36c6 to 465257d Compare August 11, 2023 09:55

roman-khimov approved these changes Aug 11, 2023

View reviewed changes

This was referenced Aug 11, 2023

Implement ReaderFrom for client.DefaultObjectWriter and slicer.PayloadWriter nspcc-dev/neofs-sdk-go#488

Open

Implement WriterTo in client.PayloadReader and client.ObjectRangeReader nspcc-dev/neofs-sdk-go#489

Open

roman-khimov merged commit 29548c7 into master Aug 14, 2023
10 of 12 checks passed

roman-khimov deleted the 260-upgrade-sdk-to-rc10 branch August 14, 2023 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade sdk to rc10 #261

Upgrade sdk to rc10 #261

smallhive commented Aug 8, 2023

codecov bot commented Aug 8, 2023 •

edited

Loading

cthulhu-rider left a comment

cthulhu-rider Aug 10, 2023

smallhive Aug 10, 2023

cthulhu-rider Aug 10, 2023

smallhive Aug 11, 2023

cthulhu-rider Aug 10, 2023

smallhive Aug 10, 2023

cthulhu-rider Aug 10, 2023

smallhive Aug 10, 2023

roman-khimov Aug 10, 2023

smallhive Aug 11, 2023

roman-khimov Aug 11, 2023

smallhive Aug 11, 2023

smallhive Aug 11, 2023

smallhive Aug 11, 2023

smallhive commented Aug 11, 2023

cthulhu-rider Aug 11, 2023

smallhive Aug 11, 2023

cthulhu-rider Aug 11, 2023

smallhive Aug 11, 2023

roman-khimov Aug 11, 2023

               		return
               	}
+              	for {

Upgrade sdk to rc10 #261

Upgrade sdk to rc10 #261

Conversation

smallhive commented Aug 8, 2023

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

cthulhu-rider 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

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

smallhive commented Aug 11, 2023

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

codecov bot commented Aug 8, 2023 •

edited

Loading