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

Switch to v2 object split scheme #957

Merged
merged 7 commits into from
Jun 14, 2024

Conversation

smallhive
Copy link
Contributor

No description provided.

@smallhive smallhive linked an issue May 28, 2024 that may be closed by this pull request
go.mod Outdated Show resolved Hide resolved
cmd/s3-gw/app_settings.go Outdated Show resolved Hide resolved
internal/neofs/tree.go Show resolved Hide resolved
api/layer/tree_service.go Show resolved Hide resolved
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from f242f22 to 0bff48e Compare May 30, 2024 07:04
api/data/tree.go Outdated Show resolved Hide resolved
api/data/tree.go Outdated Show resolved Hide resolved
api/layer/multipart_upload.go Outdated Show resolved Hide resolved
api/layer/neofs.go Outdated Show resolved Hide resolved
api/layer/neofs_mock.go Show resolved Hide resolved
api/layer/tree_mock.go Show resolved Hide resolved
api/layer/tree_service.go Show resolved Hide resolved
api/layer/neofs_mock.go Show resolved Hide resolved
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from 0bff48e to 54a811f Compare May 31, 2024 05:09
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 9.13462% with 189 lines in your changes missing coverage. Please review.

Project coverage is 26.71%. Comparing base (d6d4636) to head (e016f43).
Report is 32 commits behind head on master.

Current head e016f43 differs from pull request most recent head 8328780

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

Files Patch % Lines
api/layer/multipart_upload.go 0.00% 71 Missing ⚠️
api/layer/neofs_mock.go 0.00% 36 Missing and 1 partial ⚠️
internal/neofs/tree.go 0.00% 30 Missing ⚠️
api/data/tree.go 0.00% 16 Missing ⚠️
internal/neofs/neofs.go 0.00% 13 Missing ⚠️
api/layer/tree_mock.go 0.00% 9 Missing ⚠️
api/handler/object_list.go 12.50% 6 Missing and 1 partial ⚠️
api/handler/multipart_upload.go 0.00% 4 Missing ⚠️
cmd/s3-gw/app_settings.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
- Coverage   26.92%   26.71%   -0.21%     
==========================================
  Files          87       87              
  Lines       14129    14246     +117     
==========================================
+ Hits         3804     3806       +2     
- Misses       9903    10017     +114     
- Partials      422      423       +1     

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

@smallhive smallhive linked an issue Jun 10, 2024 that may be closed by this pull request
api/handler/multipart_upload.go Outdated Show resolved Hide resolved
api/handler/multipart_upload.go Outdated Show resolved Hide resolved
internal/neofs/tree.go Outdated Show resolved Hide resolved
internal/neofs/tree.go Show resolved Hide resolved
api/handler/object_list.go Outdated Show resolved Hide resolved
internal/neofs/tree.go Show resolved Hide resolved
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch 3 times, most recently from b7c4cba to cf43c02 Compare June 11, 2024 04:27
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from cf43c02 to c699af0 Compare June 13, 2024 13:40
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.

I've expected more red lines, but probably we're still too dependent on the tree service.

api/handler/multipart_upload.go Show resolved Hide resolved
var (
bktInfo = p.Bkt
attributes [][2]string
multipartHash = sha256.New()
Copy link
Member

Choose a reason for hiding this comment

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

Technically it's not needed, you know it has no data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is correct. Unfortunately, I failed to avoid it. Even empty data must be hashed and marshaled to be available for the next parts. This required calculating a hash for the whole object and the next part uses the "hash state" from the previous one

Closes #937.

Signed-off-by: Evgenii Baidakov <[email protected]>
In real NeoFS the complex object is constructing by the node using linking object, previous-id header, etc. For tests we use only the map and this step creates real object in this map.

Signed-off-by: Evgenii Baidakov <[email protected]>
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from c699af0 to 2931301 Compare June 14, 2024 08:09
api/handler/object_list.go Outdated Show resolved Hide resolved
api/layer/tree_service.go Show resolved Hide resolved
These changes fixes test_s3.py::test_versioning_obj_suspend_versions
and test_s3.py::test_list_multipart_upload_owner tests.

Signed-off-by: Evgenii Baidakov <[email protected]>
Fixes s3tests_boto3/functional/test_s3.py::test_encryption_sse_c_multipart_bad_download test.

Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
Current texts are misleading a bit.

Signed-off-by: Evgenii Baidakov <[email protected]>
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from 2931301 to 8328780 Compare June 14, 2024 11:59
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

This is (has been for 2 weeks) my first experience with our S3 review. Overall, looks working to me. However, I still do not understand why some changes are done now and in the replace v1 to v2 commit. We have not changed the split noticeably, it is just now more convenient and more informative for SNs, but it is still an object chain, and still, the whole chain is backward-dependent, so it is strange.

Also, reuploading parts logic blows my mind, but it is not this PR's scope.

@roman-khimov roman-khimov merged commit 0c5bdbe into master Jun 14, 2024
16 of 17 checks passed
@roman-khimov roman-khimov deleted the 937-switch-to-v2-object-split-scheme branch June 14, 2024 15:19
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.

GET of a multipart object fails Switch to v2 object split scheme
3 participants