Drop ResolveNeoFSFailures #413

smallhive · 2023-05-15T11:58:34Z

close #406
fixes #405

smallhive · 2023-05-16T11:02:18Z

@roman-khimov @cthulhu-rider @notimetoname

I want to attract your attention to one question. I'm almost done the PR, but it perfectly works only for Go1.20.
Go < 1.20 doesn't have some functions in errors package. That is why I have to make this commit to fix this.

Be honest, I'm completely not exiting about it, because we should revert/update it in future. When minimal Go version will be 1.20 it should be replaced with errors.Join here like errors.Join(ErrAPI, err)

I tend to think, I'm missing some good solution to wrap multiple errors (and correct check them with errors.Is) in Go < 1.20. May be you may suggest something about that.

How I understand setting Go 1.20 as minimum version in SDK is not an option, right?

roman-khimov · 2023-05-16T11:27:36Z

How I understand setting Go 1.20 as minimum version in SDK is not an option, right?

Yes, we can't demand it for any of our components, but especially in SDK, it can be used by random people and we can't expect them to have 1.20. So we absolutely need this compatibility code that will eventually be taken out.

notimetoname · 2023-05-16T11:32:05Z

How I understand setting Go 1.20 as minimum version in SDK is not an option, right?

Sweet dreams but dont think so.

I'm completely not exiting about it

Me too. The only thing i would request is to leave as many reminders to fix it when 1.22 is out as possible (comments, issues, links/TODOs/etc).

smallhive · 2023-05-16T11:34:08Z

The only thing i would request is to leave as many reminders to fix it when 1.23 is out as possible (comments, issues, links/TODOs/etc).

Agree, I will add them when we decide what and how we manage to this.

roman-khimov · 2023-05-16T11:35:20Z

We just need an issue to track this. And we better keep commits the way they are (using new APIs and then changing to old ones), it'd be trivial to revert then.

smallhive · 2023-05-16T11:37:23Z

We just need an issue to track this. And we better keep commits the way they are (using new APIs and then changing to old ones), it'd be trivial to revert then.

Sure, I will make it

roman-khimov

Now consider some ResObjectDelete, why does it have a statusRes inside when it's no longer being used? It can't have anything but successful value, because all the other codes are returned as errors. Then consider something like ResContainerDelete, is it needed at all with the new scheme? Let's simplify these things as well.

client/status/common.go

client/container.go

storagegroup/storagegroup_test.go

client/status/unrecognized.go

cthulhu-rider · 2023-05-16T15:17:47Z

i only had time to briefly review the proposed changes

i think apistatus package shouldn't be touched at all because it declares structured errors and that's clean. I'd leave this package passive and implement all business logic in client package - it actively returns errors (but not only, for example, storage node does not need all this functionality with wrapping errors in so-called API ones at all). client package has everything he needs to work with API errors (all types provide error, apistatus.Status, As(any) bool and Is(error) bool interfaces)

roman-khimov · 2023-05-16T16:34:24Z

i think apistatus package shouldn't be touched at all

We need some type for generic API error and as we have (almost) all of them in apistatus, it makes sense to continue using it. Otherwise we better move all of errors into client, which is an option, but a bit more invasive one. I'd stick to apistatus for now.

cthulhu-rider · 2023-05-16T16:42:56Z

Otherwise we better move all of errors into client, which is an option

not a good option to me: apistatus is also used by server side (that's why it was designed as an autonomous package). At the same time, error classification (internal, API or low-level) applies only to the client side. That's why I suggest such packaging

roman-khimov · 2023-05-16T16:53:21Z

apistatus obviously only contains NeoFS protocol errors, it's OK for it to provide some means for checking for any or a group of these errors. It can be used by any side then without any issues.

cthulhu-rider · 2023-05-16T17:16:24Z

it's OK for it to provide some means for checking for any

yes, it does, there is https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.8/client/status#Status interface provided by all protocol statuses incl. errors (although empty now, but can be extended). That's enough for any party. Network errors occur in client games only

i don't mind to have API error group in apistatus package, as for me it does not matter, although it is at odds with my vision of organizing packages

roman-khimov

See #413 (review) as well.

client/status/unrecognized.go

client/errors_test.go

client/status/status.go

client/status/v2_test.go

client/netmap.go

client/object_delete.go

client/object_get.go

client/object_hash.go

client/status/common.go

client/status/status.go

client/status/common.go

client/container.go

client/netmap_test.go

client/object_get.go

client/object_search.go

pool/pool.go

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

TestFromStatusV2 and TestToStatusV2 have the identical testcases and code to check them Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

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

roman-khimov

+583 −817 --- looks better now
Please mention #405 in commits/PR, because this patch set fixes it as well.

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

Should be reverted/updated when minimum version of Go will be set to 1.20 Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

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

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

We've got a number of `Res*` things with a single member only. They were somewhat useful before #413 (to get request status), but now they provide zero value and just complicate the code using these APIs. Notice also that this makes `client` interface a little close to `pool` interface which is nice wrt. #380.

It is possible and safe after nspcc-dev/neofs-sdk-go#413. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

smallhive force-pushed the 406-drop-resolveneofsfailures branch 3 times, most recently from 890364b to 9cba1b6 Compare May 16, 2023 10:41

smallhive force-pushed the 406-drop-resolveneofsfailures branch from 9cba1b6 to ed1ff35 Compare May 16, 2023 11:45

smallhive marked this pull request as ready for review May 16, 2023 11:49

smallhive requested review from roman-khimov, cthulhu-rider and notimetoname as code owners May 16, 2023 11:49

roman-khimov reviewed May 16, 2023

View reviewed changes

client/status/common.go Outdated Show resolved Hide resolved

client/container.go Outdated Show resolved Hide resolved

storagegroup/storagegroup_test.go Outdated Show resolved Hide resolved

roman-khimov reviewed May 16, 2023

View reviewed changes

client/status/unrecognized.go Outdated Show resolved Hide resolved

smallhive force-pushed the 406-drop-resolveneofsfailures branch from ed1ff35 to c14dc05 Compare May 17, 2023 07:24

smallhive requested a review from roman-khimov May 17, 2023 07:28

smallhive force-pushed the 406-drop-resolveneofsfailures branch from c14dc05 to 4ef4ffe Compare May 17, 2023 08:32

roman-khimov reviewed May 17, 2023

View reviewed changes

smallhive force-pushed the 406-drop-resolveneofsfailures branch 2 times, most recently from d876fb2 to 516df0a Compare May 17, 2023 09:52

smallhive requested a review from roman-khimov May 17, 2023 10:27

smallhive force-pushed the 406-drop-resolveneofsfailures branch from 516df0a to ed1536f Compare May 17, 2023 10:37

cthulhu-rider reviewed May 17, 2023

View reviewed changes

client/status/common.go Outdated Show resolved Hide resolved

roman-khimov reviewed May 18, 2023

View reviewed changes

smallhive force-pushed the 406-drop-resolveneofsfailures branch 4 times, most recently from 67af63a to e07c8fb Compare May 19, 2023 06:14

smallhive added 4 commits May 19, 2023 10:33

client: Add separate error for each apistatus

fc0bd12

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

client: Move and extend status errors tests

9533a77

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

client: Remove duplicate test function

483aff3

TestFromStatusV2 and TestToStatusV2 have the identical testcases and code to check them Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

client: Replace apistatus type check with error check

1d952ce

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

smallhive force-pushed the 406-drop-resolveneofsfailures branch from e07c8fb to 95ecf9f Compare May 19, 2023 06:33

cthulhu-rider approved these changes May 19, 2023

View reviewed changes

smallhive requested a review from roman-khimov May 19, 2023 08:02

roman-khimov reviewed May 19, 2023

View reviewed changes

roman-khimov mentioned this pull request May 19, 2023

Revise apistatus ErrFromStatus/ErrToStatus/IsSuccessful #417

Closed

smallhive added 8 commits May 19, 2023 12:19

client: Extend tests for apistatuses

149b145

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

client: Update documentation

548f911

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

client: Remove ResolveNeoFSFailures

2f45caf

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

client: Fix linter - unnecessary leading newline

9e10797

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

client: Fix linter - unused parameter

a4e14ab

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

client: Fix error checking for go 1.18-19

e0afe08

Should be reverted/updated when minimum version of Go will be set to 1.20 Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>

*: Remove statusRes as unused

e377b3b

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

pool: Refactor handleError to updateErrorRate

49bc3b7

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

smallhive force-pushed the 406-drop-resolveneofsfailures branch from a91a957 to 49bc3b7 Compare May 19, 2023 08:25

roman-khimov approved these changes May 19, 2023

View reviewed changes

notimetoname approved these changes May 19, 2023

View reviewed changes

roman-khimov merged commit cfdd870 into master May 19, 2023

roman-khimov deleted the 406-drop-resolveneofsfailures branch May 19, 2023 09:13

roman-khimov mentioned this pull request May 25, 2023

Strip useless res structs #426

Merged

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Oct 1, 2024

shard/errors: do not use pointers and As if not needed

14c7426

It is possible and safe after nspcc-dev/neofs-sdk-go#413. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop ResolveNeoFSFailures #413

Drop ResolveNeoFSFailures #413

smallhive commented May 15, 2023 •

edited

Loading

smallhive commented May 16, 2023

roman-khimov commented May 16, 2023

notimetoname commented May 16, 2023 •

edited

Loading

smallhive commented May 16, 2023

roman-khimov commented May 16, 2023

smallhive commented May 16, 2023

roman-khimov left a comment

cthulhu-rider commented May 16, 2023

roman-khimov commented May 16, 2023

cthulhu-rider commented May 16, 2023

roman-khimov commented May 16, 2023

cthulhu-rider commented May 16, 2023 •

edited

Loading

roman-khimov left a comment

roman-khimov left a comment

Drop ResolveNeoFSFailures #413

Drop ResolveNeoFSFailures #413

Conversation

smallhive commented May 15, 2023 • edited Loading

smallhive commented May 16, 2023

roman-khimov commented May 16, 2023

notimetoname commented May 16, 2023 • edited Loading

smallhive commented May 16, 2023

roman-khimov commented May 16, 2023

smallhive commented May 16, 2023

roman-khimov left a comment

Choose a reason for hiding this comment

cthulhu-rider commented May 16, 2023

roman-khimov commented May 16, 2023

cthulhu-rider commented May 16, 2023

roman-khimov commented May 16, 2023

cthulhu-rider commented May 16, 2023 • edited Loading

roman-khimov left a comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

smallhive commented May 15, 2023 •

edited

Loading

notimetoname commented May 16, 2023 •

edited

Loading

cthulhu-rider commented May 16, 2023 •

edited

Loading