Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

go.mod: Upgrade github.com/nspcc-dev/neofs-sdk-go #3199

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

go.mod: Upgrade github.com/nspcc-dev/neofs-sdk-go #3199

wants to merge 8 commits into from

Conversation

cthulhu-rider
Copy link
Contributor

asserted the fix with the "big" object

to apply it, we need to upgrade SDK. It pulls Go 1.23 requirement. Current module hasn't bumped the min Go yet #2918. It's currently scheduled for v0.46.0. We can consider inclusion in v0.45.1. @roman-khimov @carpawell

other ways without updating the SDK seem practically impossible to me at the moment. This tie is generally inconvenient, although the node keeps up with the SDK

@codecov Codecov
Copy link

codecov bot commented Mar 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 14.58333% with 82 lines in your changes missing coverage. Please review.

Project coverage is 22.15%. Comparing base (5a9e733) to head (e875243).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...rs/netmap/nodevalidation/availability/validator.go 0.00% 9 Missing ⚠️
pkg/services/audit/auditor/pop.go 0.00% 9 Missing ⚠️
pkg/network/cache/multi.go 0.00% 7 Missing ⚠️
internal/testutil/rand.go 0.00% 6 Missing ⚠️
pkg/services/tree/redirect.go 0.00% 6 Missing ⚠️
pkg/services/tree/sync.go 0.00% 6 Missing ⚠️
cmd/neofs-node/transport.go 0.00% 4 Missing ⚠️
pkg/network/group.go 66.66% 4 Missing ⚠️
pkg/services/tree/replicator.go 0.00% 4 Missing ⚠️
pkg/services/object/put/streamer.go 0.00% 3 Missing ⚠️
... and 19 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
- Coverage   23.42%   22.15%   -1.27%     
==========================================
  Files         760      761       +1     
  Lines       60848    60745     -103     
==========================================
- Hits        14252    13460     -792     
- Misses      45588    46301     +713     
+ Partials     1008      984      -24

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

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@carpawell
Copy link
Member

I have no problems with updating go.mod without closing #2918 and applying additional changes via a separate PR. Also, I think it is not critical for this PR to wait before #2918 is fixed.

@roman-khimov
Copy link
Member

We don't need Go 1.23 for 0.45.1.

@cthulhu-rider
Copy link
Contributor Author

I dont care about go much here, I want this fix to be released asap

roman-khimov
roman-khimov reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But otherwise OK. Even though I'm not excited about these changes (they better be for 0.46.0) we are using newer Go anyway already and we need to fix the problem. Workflows will fail though, so some adjustments are needed there.

@roman-khimov
Copy link
Member

Looks like we're not ready for this one. Let's roll out 0.45.1 with this known bug and upgrade Go and SDK properly then.

@cthulhu-rider
go.mod: Bump min required Go to 1.23
f95feb3 
Update GitHub workflows and Docker builders too.

Refs #2918.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
*: Do not stop timers and tickers for GC
de19721 
With Go 1.23 which is min required since f95feb3,
`Stop` is no longer required to be called for `Timer`/`Ticker` resource
collecting.

Refs #2918.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
*: Use built-in clear function to clear maps
a46c4a0 
It's available starting from Go 1.21 release.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
*: Replace deprecated math/rand.Read with ChaCha8
5396b33 
With Go1.23 which is min required since f95feb3,
`rand/ChaCha8.Read` method is now available. It covers all current
testing purposes.

New `testutil` package collecting various Go test helpers has also been
created.

Refs ##2918.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
Drop redundant string([]byte) conversion
25ef7d0 
Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider marked this pull request as ready for review March 7, 2025 14:37
@cthulhu-rider cthulhu-rider requested a review from carpawell as a code owner March 7, 2025 14:37
@cthulhu-rider cthulhu-rider force-pushed the bugfix/zero-range branch 2 times, most recently from 2cd2f5e to 5059b3c Compare March 10, 2025 09:48
@cthulhu-rider cthulhu-rider marked this pull request as draft March 10, 2025 10:17
@cthulhu-rider cthulhu-rider marked this pull request as ready for review March 10, 2025 12:07
roman-khimov
roman-khimov approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New iterators look good in many cases, simplifying flow, especially for early returns and things like that. But nspcc-dev/neofs-sdk-go#693 will certainly need to be fixed, current code around IterateNetworkEndpoints is far from perfect.

cmd/neofs-adm/internal/modules/fschain/dump_names.go Outdated
@@ -70,7 +71,7 @@ func dumpNames(cmd *cobra.Command, _ []string) error {
return len(iParts) < len(jParts)
}
for k := len(iParts) - 1; k >= 0; k-- {
var c = strings.Compare(iParts[k], jParts[k])
var c = cmp.Compare(iParts[k], jParts[k])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 1.23+ strings.Compare is the same as cmp.Compare performance-wise. This can be omitted, less packages to import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped this commit

@cthulhu-rider
*: Replace some deprecated code
c18a654 
Only NeoFS SDK code which can be replaced trivially.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
go.mod: Upgrade github.com/nspcc-dev/neofs-sdk-go to RC13
3b853d5 
The latest at the moment. No code changes involved. Also pulls Neo Go
v0.108.1 upgrade and some others.

Pre-release includes fix of `client/Client.ObjectRangeInit` method for
zero range case. The bug first encountered while using `neofs-cli
object range` cmd:
```
can't get object payload range: copy payload: payload range size overflow
```

This also prevents similar problems in inter-node communications while
serving `ObjectService.Range` RPC.

Upgrade of google.golang.org/grpc to v1.70.0 required moving to codec V2
usage. Similar to nspcc-dev/neofs-sdk-go@b7f8cd0.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
*: Use for-range loops over iterator functions
e875243 
Possible with Go 1.23 which is min required since f95feb3.

Closes #2918.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@carpawell carpawell Awaiting requested review from carpawell carpawell is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cthulhu-rider @carpawell @roman-khimov