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

Blobstor: drop the last prm and res #3039

Merged
merged 13 commits into from
Dec 6, 2024
Merged

Blobstor: drop the last prm and res #3039

merged 13 commits into from
Dec 6, 2024

Conversation

roman-khimov
Copy link
Member

No description provided.

@roman-khimov roman-khimov added this to the v0.45.0 milestone Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 69.59459% with 90 lines in your changes missing coverage. Please review.

Project coverage is 22.41%. Comparing base (2d77475) to head (d788198).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
pkg/local_object_storage/blobstor/fstree/fstree.go 72.54% 13 Missing and 1 partial ⚠️
cmd/neofs-lens/internal/peapod/get.go 0.00% 10 Missing ⚠️
pkg/local_object_storage/blobstor/peapod/peapod.go 77.27% 9 Missing and 1 partial ⚠️
pkg/local_object_storage/blobstor/delete.go 0.00% 9 Missing ⚠️
...kg/local_object_storage/blobstor/common/storage.go 36.36% 4 Missing and 3 partials ⚠️
pkg/local_object_storage/blobstor/get_range.go 0.00% 7 Missing ⚠️
pkg/local_object_storage/blobstor/exists.go 54.54% 4 Missing and 1 partial ⚠️
pkg/local_object_storage/blobstor/get.go 50.00% 4 Missing ⚠️
pkg/local_object_storage/blobstor/iterate.go 55.55% 2 Missing and 2 partials ⚠️
pkg/local_object_storage/metabase/put.go 40.00% 3 Missing ⚠️
... and 12 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3039      +/-   ##
==========================================
- Coverage   22.62%   22.41%   -0.21%     
==========================================
  Files         791      791              
  Lines       58615    58426     -189     
==========================================
- Hits        13261    13096     -165     
+ Misses      44455    44435      -20     
+ Partials      899      895       -4     

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

@roman-khimov
Copy link
Member Author

@roman-khimov
Copy link
Member Author

Buggy, to be fixed.

info log/log.go:12 local object storage operation {"shard_id": "CAXqYYjUEAbdr8gcLFgrpa", "address": "56b9fmqewnWFfShD9SmKF5GzB4tpmGdfR9acgCftuejk/8yZJAhrohFoNY1DTyHSzhCYTZozP1TzaPwgjokvk2zZu", "op": "metabase PUT"}
2024/12/02 20:01:33 init shard CAXqYYjUEAbdr8gcLFgrpa: could not initialize *shard.metabaseSynchronizer: could not put objects to the meta from blobstor: blobstor iterator failure: exec read-only BoltDB transaction: logical error: no split info on parent object

@roman-khimov roman-khimov force-pushed the blobstor-prm-res branch 2 times, most recently from 1566328 to c0d303c Compare December 3, 2024 12:19
@roman-khimov
Copy link
Member Author

if !prm.DontCompress {
prm.RawData = t.Compress(prm.RawData)
}
prm.RawData = t.Compress(prm.RawData)
Copy link
Member

Choose a reason for hiding this comment

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

hm, a confusing change imo. suggest renaming to CompressIfEnabled

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the way it's named now, I'd not bother.

Copy link
Member

Choose a reason for hiding this comment

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

previously it was near if !prm.DontCompress {, now it just always compresses

Copy link
Member Author

Choose a reason for hiding this comment

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

That didn't change anything in fact, prm.DontCompress was an external signal, while t.Compress can always do whatever it wants to internally.

One thing that bugs me is that our blobstor is not really a BLOB storage system, it handles compression at the storage level (why?), it deals with objects for whatever reason. Would be much nicer to see some []byte in/out here and handle compression at engine level. But that's all a different story.

At the substorage-level we don't need IDs, at the blobstor level we still do.

Signed-off-by: Roman Khimov <[email protected]>
Simplify interfaces.

Signed-off-by: Roman Khimov <[email protected]>
Get() is an object-based interface, while GetBytes() is for raw data.

Signed-off-by: Roman Khimov <[email protected]>
It's easier this way.

Signed-off-by: Roman Khimov <[email protected]>
This splits the lazy interface in a separate method, but otherwise works
the same.

Signed-off-by: Roman Khimov <[email protected]>
No one cares about it, dropping it simplifies refactoring.

Signed-off-by: Roman Khimov <[email protected]>
Storage IDs make zero sense at the fstree/peapod level, so they're ommitted
there and otherwise the behavior is preserved. Blobstor also currently accepts
object structures, although it may not really care about them.

Signed-off-by: Roman Khimov <[email protected]>
No reason for it to be a function.

Signed-off-by: Roman Khimov <[email protected]>
I believe this bug was introduced right with 7df5029,
this code is a straight copy-paste from bbcz, but bbcz operates in a callback,
so it must return anyway and if errorHandler returns non-nil value is stops
the iteration. In FSTree this leads to iteration stopping in any event if
errorHandler is invoked and this shouldn't be the case.

Signed-off-by: Roman Khimov <[email protected]>
Which simplifies things a bit for external users. If errorHandler is not
specified any errors are returned as is, if it is it's up to handler to
decide.

Signed-off-by: Roman Khimov <[email protected]>
Address decoding error can lead to incorrect address (non-null) being passed
to errorHandler. It's safer to have more narrow scope for addr.

Signed-off-by: Roman Khimov <[email protected]>
Object getter callback is useless for FSTree and a bit more efficient for
Peapod. But the only user of this interface is writecache which only uses it
with FSTree, so some deduplication can be helpful.

Signed-off-by: Roman Khimov <[email protected]>
Fix resynchronization failure:

   2024/12/02 20:01:33 init shard CAXqYYjUEAbdr8gcLFgrpa: could not initialize *shard.metabaseSynchronizer: could not put objects to the meta from blobstor: blobstor iterator failure: exec read-only BoltDB transaction: logical error: no split info on parent object

In general, exists() call shouldn't affect Put() much.

Signed-off-by: Roman Khimov <[email protected]>
@roman-khimov roman-khimov merged commit 07cea01 into master Dec 6, 2024
22 checks passed
@roman-khimov roman-khimov deleted the blobstor-prm-res branch December 6, 2024 08:52
@roman-khimov roman-khimov modified the milestones: v0.45.0, v0.44.1 Dec 11, 2024
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.

2 participants