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

Fix/GC big objects parts #2863

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Fix/GC big objects parts #2863

merged 3 commits into from
Jun 13, 2024

Conversation

carpawell
Copy link
Member

Closes #2858.

@carpawell carpawell self-assigned this Jun 10, 2024
@codecov Codecov
Copy link

codecov bot commented Jun 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 38.33333% with 37 lines in your changes missing coverage. Please review.

Project coverage is 23.69%. Comparing base (e5da072) to head (31b9074).
Report is 4 commits behind head on master.

❗ Current head 31b9074 differs from pull request most recent head 658cbde

Please upload reports for the commit 658cbde to get more accurate results.

Files Patch % Lines
pkg/services/replicator/process.go 0.00% 15 Missing ⚠️
pkg/local_object_storage/shard/exists.go 0.00% 3 Missing and 1 partial ⚠️
pkg/local_object_storage/shard/gc.go 42.85% 4 Missing ⚠️
pkg/local_object_storage/shard/shard.go 0.00% 3 Missing ⚠️
pkg/services/replicator/replicator.go 0.00% 3 Missing ⚠️
pkg/core/object/object.go 66.66% 1 Missing and 1 partial ⚠️
pkg/local_object_storage/engine/inhume.go 71.42% 1 Missing and 1 partial ⚠️
pkg/local_object_storage/metabase/exists.go 60.00% 2 Missing ⚠️
cmd/neofs-node/object.go 0.00% 1 Missing ⚠️
pkg/core/object/fmt.go 87.50% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   23.56%   23.69%   +0.12%     
==========================================
  Files         773      770       -3     
  Lines       44672    44540     -132     
==========================================
+ Hits        10529    10555      +26     
+ Misses      33290    33131     -159     
- Partials      853      854       +1

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

@carpawell
Copy link
Member Author

Found corner case in shard distribution, will add tests and fix.

@carpawell carpawell marked this pull request as draft June 10, 2024 10:58
@carpawell
Copy link
Member Author

carpawell commented Jun 10, 2024 •
edited
Loading

OK, a fresh view and an extended test showed that it did not work. Let it be simple and error-proof.

@carpawell carpawell marked this pull request as ready for review June 10, 2024 22:37
This was referenced Jun 10, 2024
Open
Closed
cthulhu-rider
cthulhu-rider reviewed Jun 11, 2024
View reviewed changes
pkg/core/object/object.go Outdated Show resolved Hide resolved
pkg/local_object_storage/engine/engine_test.go Outdated Show resolved Hide resolved
pkg/services/replicator/process.go Outdated Show resolved Hide resolved
pkg/services/replicator/process.go Outdated Show resolved Hide resolved
pkg/services/replicator/process.go Outdated Show resolved Hide resolved
pkg/services/replicator/process.go Outdated Show resolved Hide resolved
@carpawell
object: log expiration attribute if check fails
ddb1dd8 
We have found a deadlock replication cycle; numbers are good things to know in
this case.

Signed-off-by: Pavel Karpy <[email protected]>
@carpawell
gc: handle expired children parts
c707716 
Closes #2858.

Signed-off-by: Pavel Karpy <[email protected]>
@carpawell
core/object: make expiration searching routine public
658cbde 
Many packages may benefit from it.

Signed-off-by: Pavel Karpy <[email protected]>
roman-khimov
roman-khimov reviewed Jun 13, 2024
View reviewed changes
pkg/core/object/object.go

// Expiration searches for expiration attribute in the object. Returns
// ErrNoExpiration if not found.
func Expiration(obj object.Object) (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this if we're not touching replicator?

Copy link
Member Author

Choose a reason for hiding this comment

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

this can be useful in general as i mentioned in the commit, i just had not searched if more places can reused it

@roman-khimov roman-khimov merged commit 690c64d into master Jun 13, 2024
19 of 22 checks passed
@roman-khimov roman-khimov deleted the fix/gc-big-objects-parts branch June 13, 2024 13:41
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

@cthulhu-rider cthulhu-rider cthulhu-rider approved these changes

Assignees

@carpawell carpawell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GC has not collected an expired object
3 participants
@carpawell @roman-khimov @cthulhu-rider