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

Cache storage policy application results (Put) #2901

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Cache storage policy application results (Put) #2901

merged 2 commits into from
Aug 30, 2024

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider changed the title object/put: Refactor storage policy processing Cache storage policy application results (Put) Jul 29, 2024
@cthulhu-rider cthulhu-rider force-pushed the optimize/put-policy branch 2 times, most recently from 3e65761 to 00e3d64 Compare August 9, 2024 16:10
@cthulhu-rider cthulhu-rider marked this pull request as ready for review August 9, 2024 16:11
@codecov Codecov
Copy link

codecov bot commented Aug 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 63.09524% with 93 lines in your changes missing coverage. Please review.

Project coverage is 23.91%. Comparing base (7baa16c) to head (2bb75dd).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
cmd/neofs-node/object.go 0.00% 47 Missing ⚠️
pkg/services/object/put/streamer.go 0.00% 25 Missing ⚠️
pkg/services/object/put/distributed.go 88.88% 14 Missing and 4 partials ⚠️
pkg/services/object/put/service.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2901      +/-   ##
==========================================
+ Coverage   23.51%   23.91%   +0.40%     
==========================================
  Files         776      775       -1     
  Lines       45325    45648     +323     
==========================================
+ Hits        10656    10917     +261     
- Misses      33822    33874      +52     
- Partials      847      857      +10

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

carpawell
carpawell reviewed Aug 13, 2024
View reviewed changes
cmd/neofs-node/object.go
@@ -256,14 +256,13 @@ func initObjectService(c *cfg) {
searchsvcV2.WithKeyStorage(keyStorage),
)

sPut := putsvc.NewService(&transport{clients: putConstructor},
sPut := putsvc.NewService(&transport{clients: putConstructor}, c,
Copy link
Member

Choose a reason for hiding this comment

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

btw, why not an option? looks strange when there are position args and options but both are required in fact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to not forget to pass it. I agree that in the end this should be brought to a general form

cmd/neofs-node/object.go
// policy and returns sort interface.
//
// GetContainerNodes implements [putsvc.NeoFSNetwork].
func (c *cfg) GetContainerNodes(cnrID cid.ID) (putsvc.ContainerNodes, error) {
Copy link
Member

Choose a reason for hiding this comment

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

why isn't it a part of containerNodes? our cfg becomes too complex. this method only uses cfg's c.cfgObject.containerNodes

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah this would be nice, but cfg does not inherit containerNodes and providing putsvc.NeoFSNetwork from containerNodes is not intended right now. I'll try to do smth about it

UPD: this would require either one more interface adapter or separate interface for key locality check (now it's a single NeoFSNetwork). Current version with the least changes

pkg/services/object/put/distributed.go Outdated Show resolved Hide resolved
pkg/services/object/put/distributed.go Show resolved Hide resolved
pkg/services/object/put/distributed.go Show resolved Hide resolved
@cthulhu-rider cthulhu-rider force-pushed the optimize/put-policy branch 2 times, most recently from 9f1d7ef to 3084ed5 Compare August 19, 2024 13:23
@cthulhu-rider
object/put: Refactor storage policy processing
5076fe2 
Continues 2f29338 for `ObjectService`'s
`Put` server handler. opens the stage for optimizations of storage policy
imposition. Previously, node selection was hidden behind placement traverser
interface. currently, processing a single request requires preselection of
container nodes in unsorted form and, in general w/ slicing, several sorts to
place the resulting objects. In the previous code structure, cache optimization
would be less seamless. Also, this refactor reveals the entire placement logic
by removing unnecessary abstraction.

Implementation is pretty massive, so unit tests for various scenarios are
added.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@carpawell
Copy link
Member

https://rest.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/1842-1724261309/index.html

Something is not good with this PR.

@roman-khimov
Copy link
Member

Except CHANGELOG needs to be updated.

@cthulhu-rider
node/object: Cache object policy application results for Put service
2bb75dd 
Continues 5389a1e for `ObjectService`'s
`Put` server handler. It shares the cache with other object processors.

Refs #1803.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@roman-khimov roman-khimov merged commit 35d3c67 into master Aug 30, 2024
22 checks passed
@roman-khimov roman-khimov deleted the optimize/put-policy branch August 30, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carpawell carpawell carpawell approved these changes

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

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