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

Do not use github.com/nspcc-dev/neofs-api-go/v2 where unnecessary #3057

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from
Draft

Do not use github.com/nspcc-dev/neofs-api-go/v2 where unnecessary #3057

wants to merge 21 commits into from

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Dec 18, 2024 •
edited
Loading

self-reminder: check refs in commit messages before merging

a lot of changes, but this will maximally amortize coming update to the latest SDK

@codecov Codecov
Copy link

codecov bot commented Dec 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 11.16679% with 2482 lines in your changes missing coverage. Please review.

Project coverage is 22.92%. Comparing base (7ac9947) to head (fb1fd87).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
pkg/services/object/server.go 10.23% 1339 Missing and 12 partials ⚠️
pkg/services/container/server.go 24.55% 240 Missing and 12 partials ⚠️
pkg/services/object/acl/v2/service.go 0.00% 158 Missing ⚠️
cmd/neofs-node/container.go 0.00% 104 Missing ⚠️
pkg/services/netmap/server.go 0.00% 95 Missing ⚠️
pkg/services/object/acl/eacl/v2/headers.go 19.48% 59 Missing and 3 partials ⚠️
cmd/neofs-node/reputation.go 0.00% 54 Missing ⚠️
pkg/services/session/server.go 0.00% 54 Missing ⚠️
pkg/services/accounting/server.go 0.00% 52 Missing ⚠️
cmd/neofs-node/object.go 0.00% 51 Missing ⚠️
... and 28 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3057      +/-   ##
==========================================
+ Coverage   22.36%   22.92%   +0.56%     
==========================================
  Files         793      747      -46     
  Lines       58713    57115    -1598     
==========================================
- Hits        13130    13093      -37     
+ Misses      44682    43134    -1548     
+ Partials      901      888      -13

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

@cthulhu-rider cthulhu-rider force-pushed the avoid-apigo branch 9 times, most recently from a369d5e to 9aab13c Compare December 25, 2024 09:49
@cthulhu-rider
Copy link
Contributor Author

cthulhu-rider commented Dec 25, 2024 •
edited
Loading

added temp commit "fixing" base branch to run tests manually. Results:

  1. container PASS
  2. object
FAILED pytest_tests/tests/object/test_object_api.py::TestObjectApi::test_object_parts_cannot_be_deleted - AssertionError: Regex pattern did not match.
FAILED pytest_tests/tests/object/test_object_lock.py::TestObjectLockWithGrpc::test_expired_object_should_be_deleted_after_locks_are_expired[complex object] - Failed: DID NOT RAISE <class 'Exception'>
  1. acl PASS
  2. failovers
FAILED pytest_tests/tests/failovers/test_failover_network.py::TestFailoverNetwork::test_block_storage_node_traffic - AttributeError: 'TimeoutExpired' object has no attribute 'strerror'
  1. contracts PASS
  2. load
FAILED pytest_tests/tests/load/test_load.py::TestLoad::test_custom_load - RuntimeError: Command: ../xk6-neofs/scenarios/preset/preset_grpc.py --size 1024  --containers 1 --out grpc.json --endpoint localhost:46765 --preload_obj 100 --policy "REP 2 IN X CBF 1 SELECT 2 FROM * AS X" --wallet ../xk6-neofs/xk6_wallet_jazdzmwfxq --confi...
============================================================================================================== 1 failed, 1 warning in 156.55s (0:02:36) ==

@cthulhu-rider
cli/control: Do not use API client from NeoFS SDK
7af817b 
It makes no sense to create client just get underlying connection from
it. Instead, direct gRPC dial should be used.

This inlines `Client.Dial` to keep previous behavior as much as possible
(some error texts could change only). URI parser is copy-pasted because
it's internalized by the SDK. Even if it'd export it, we are not ready
for the upgrade to the latest yet.

Starting from here, `cmd/neofs-cli` packages (all CLI apps actually) no
longer uses `github.com/nspcc-dev/neofs-api-go/v2` directly which is
good considering the upcoming abandonment of it.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
cli/control: Do not lose errors
933c9ab 
Regression from 9ac84e3. Detected by
the linter:
```
cmd/neofs-cli/modules/control/shards_dump.go:65:2    ineffassign  ineffectual assignment to err
cmd/neofs-cli/modules/control/synchronize_tree.go:41:6 ineffassign  ineffectual assignment to err
```

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
*: Avoid importing github.com/nspcc-dev/neofs-api-go/v2/rpc packages
7fa03a5 
This refactors storage node app making it to use gRPC connection to
remote nodes without wrappers. The behavior is preserved with some error
text added/changed for the better. The only place left will naturally
be gone with the next SDK upgrade.

One step closer to `github.com/nspcc-dev/neofs-api-go/v2` module
deprecation.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
storage/shard: Use imported constant from SDK lib, not api-go one
73d7ba0 
This was the only direct importer of to-be-deprecated `neofs-api-go/v2`
module `pkg/local_object_storage` space.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
node: Replace code from pkg/network/transport to pkg/services
f32e23a 
This will simplify subsequent refactoring with the abandonment of
`github.com/nspcc-dev/neofs-api-go/v2` module: gRPC handlers are
going to completely replace current `Server` interface definitions from
`pkg/services` space.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider cthulhu-rider force-pushed the avoid-apigo branch 5 times, most recently from 8850179 to 56c7d67 Compare December 27, 2024 12:06
@cthulhu-rider
pkg/services: Get rid of recurrent processing
f1589e4 
This inlines components previously defined as neofs-api-go `Server`
interfaces wrapping each other one-by-one into the gRPC server handlers.
The object service is big, and kept for now: it's going to be changed as
well soon.

In addition to reducing packages, components and code, this will make it
easier to deprecate `github.com/nspcc-dev/neofs-api-go/v2` module in the
near future.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
node/metrics: Refactor stat submitting in object ops
f58b461 
Use op enum to have one method for all ops. Submit success and duration
in one call.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
node/metrics: Make method used inside package only unexported
06db86d 
It's invisible outside anyway.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline metric service
a48b7e8 
Continues b6fdbcc. This inlines one
level of recurrent object services into the gRPC server implementation.
The behavior is kept.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline signing service
2e96580 
Continues f18a8ed. Now sign service is
gone.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Refactor dependencies
e17ae1d 
Split the node abstraction into FS chain and local storage ones. The
service works only with these two root subsystems, so increasing the
number of abstractions is not expected, while the structure of the
real system is reflected better.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline service setting response meta headers
ab7a2fb 
Continues f32a3b8. Now the service is
gone.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline service checking node maintenance
c759e4f 
Continues a5902bb.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
service/object: Inline ACL service
8051803 
Continues 4d60cee.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
pkg/services/acl: Use constants from SDK
84fec64 
This will simplify neofs-api-go module deprecation.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
service/object: Inline transport splitting service
e412efc 
Continues 976e8b2.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline intermediate DELETE service
5702d14 
Continues 4ed7f5f. Other ops are going
to be changed the same way in future commits.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline intermediate SEARCH service
424f596 
Continues 6c09882.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline intermediate PUT service
76b6f4a 
Continues f36289d.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline intermediate GET/RANGE/HEAD services
8d89ebe 
Continues 068d746. All these ops are
very similar, so refactored in one scope.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
services/object: Inline intermediate HASH service
fb1fd87 
Continues 5d69dae. This completes
elimination of the recurrent architecture of object requests' handling.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
Copy link
Contributor Author

since the main branch is fixed, ive run tests here https://github.com/nspcc-dev/neofs-node/actions/runs/12515101826

@cthulhu-rider cthulhu-rider marked this pull request as ready for review December 27, 2024 13:38
@roman-khimov roman-khimov mentioned this pull request Dec 31, 2024
Open
roman-khimov
roman-khimov reviewed Dec 31, 2024
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.

  1. I've looked at the first two patches, they're OK, please move them into a separate PR.
  2. This PR does much more than needed and it's a BAD thing. We need to go step by step with thing, api-go deprecation first and then all of the other things separately. Not that they're bad, they're mostly for good, but pushing it all into a single PR slows us down considerably. Real team work can't be effective in a setting where you have unexpected massive changes floating around.

cmd/neofs-cli/modules/control/util.go Show resolved Hide resolved
@cthulhu-rider
Copy link
Contributor Author

ok, i'll split changes into several PRs

@cthulhu-rider cthulhu-rider marked this pull request as draft January 9, 2025 07:29
This was referenced Jan 9, 2025
Merged
Merged
Merged
Merged
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 left review comments

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

At least 1 approving review is required to merge this pull request.

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

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