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.

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

Fix/drop control call #2873

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Fix/drop control call #2873

merged 3 commits into from
Jul 1, 2024

Conversation

carpawell
Copy link
Member

@carpawell carpawell commented Jun 18, 2024

Overall, should close #2822, hard to say, there were many commits merged about it, and "problem" objects are already dropped.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 30.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 23.61%. Comparing base (450c4e5) to head (932e7b0).
Report is 4 commits behind head on master.

Files Patch % Lines
pkg/local_object_storage/metabase/delete.go 27.77% 10 Missing and 3 partials ⚠️
pkg/local_object_storage/engine/delete.go 33.33% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2873      +/-   ##
==========================================
- Coverage   23.69%   23.61%   -0.08%     
==========================================
  Files         770      770              
  Lines       44580    44524      -56     
==========================================
- Hits        10562    10514      -48     
+ Misses      33163    33160       -3     
+ Partials      855      850       -5     

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

pkg/local_object_storage/metabase/delete.go Outdated Show resolved Hide resolved

for _, addr := range res.AddressList() {
inhumePrm.MarkAsGarbage(addr)
_, err := e.inhumeAddr(prm.addr, inhumePrm)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it was a bit more efficient previously with sorted shards.

Copy link
Member Author

Choose a reason for hiding this comment

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

why do you think it is unsorted now?

Copy link
Member

Choose a reason for hiding this comment

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

        e.iterateOverUnsortedShards(func(sh hashedShard) (stop bool) {

in inhumeAddr

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean this? it is root object handling, there is no information about link and last objects that can be acquired via root object's ID


var splitErr *objectSDK.SplitInfoError
if !errors.As(err, &splitErr) {
if !shard.IsErrNotFound(err) {
Copy link
Member

Choose a reason for hiding this comment

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

This one differs from the two implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that is the key of my suggestion: why do we differ this logic? we do the same things (and one may always have more or less num of bugs) when delete/inhume objects, why some of them should be collected (if an object is a big one) one way while the other should be handled differently? about this PR: i have improved and fixed a little Inhume some time ago and could not understand why i had do it again one more time when looking at Delete

pkg/local_object_storage/metabase/delete.go Show resolved Hide resolved
Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

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

5b6b5a4 explains its changes by 6bf6984, maybe just squash them then?

pkg/local_object_storage/engine/delete.go Show resolved Hide resolved
pkg/local_object_storage/metabase/delete.go Show resolved Hide resolved
pkg/local_object_storage/engine/delete.go Outdated Show resolved Hide resolved
pkg/local_object_storage/metabase/delete.go Outdated Show resolved Hide resolved
pkg/local_object_storage/metabase/delete.go Outdated Show resolved Hide resolved
`Inhume` has expanding children functionality since 26704f9.
Collecting objects in every method with its own code is error-prone, also
`Delete` and `Inhume` are almost the same now. Relates #2822.

Signed-off-by: Pavel Karpy <[email protected]>
Children handling has been moved on the engine layer, it searches for links and
deletes all the children. There is no need to try it on a layer that even may
not have such objects at all (they can be moved, deleted and replicated, etc.).
Relates #2822.

Signed-off-by: Pavel Karpy <[email protected]>
@carpawell
Copy link
Member Author

5b6b5a4 explains its changes by 6bf6984, maybe just squash them then?

I prefer a couple of commits. The first one makes Delete work as Inhume, and the second drops no-op code since the previous change. IMO, they are read better this way.

@cthulhu-rider
Copy link
Contributor

the second drops no-op code

i thought that Delete code became less optimal after the 1st. If just no-op, then yeah - separate commits are much better

@cthulhu-rider
Copy link
Contributor

@carpawell worth changelog? the issue occurred in the pub network

@carpawell
Copy link
Member Author

@carpawell worth changelog? the issue occurred in the pub network

Not sure I can find correct words about the whole PR. But if you are about the original issue, we may add it, yes.

@cthulhu-rider cthulhu-rider merged commit 9a1c5e2 into master Jul 1, 2024
19 of 22 checks passed
@cthulhu-rider cthulhu-rider deleted the fix/drop-control-call branch July 1, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropping object via control service leaves traces in the metabase
3 participants