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

Switch to v2 object split scheme #957

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Switch to v2 object split scheme #957

merged 7 commits into from
Jun 14, 2024

Conversation

smallhive
Copy link
Contributor

No description provided.

@smallhive smallhive requested a review from roman-khimov as a code owner May 28, 2024 11:12
@smallhive smallhive mentioned this pull request May 28, 2024
Closed
@smallhive smallhive linked an issue May 28, 2024 that may be closed by this pull request
GET of a multipart object fails #954
Closed
@roman-khimov roman-khimov requested a review from carpawell May 29, 2024 11:55
roman-khimov
roman-khimov reviewed May 29, 2024
View reviewed changes
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from f242f22 to 0bff48e Compare May 30, 2024 07:04
carpawell
carpawell reviewed May 30, 2024
View reviewed changes
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from 0bff48e to 54a811f Compare May 31, 2024 05:09
@codecov Codecov
Copy link

codecov bot commented Jun 5, 2024 •
edited
Loading

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
Switch to v2 object split scheme #937
Closed
roman-khimov
roman-khimov reviewed Jun 10, 2024
View reviewed changes
@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 requested a review from roman-khimov June 11, 2024 04:30
@smallhive smallhive mentioned this pull request Jun 13, 2024
Closed
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from cf43c02 to c699af0 Compare June 13, 2024 13:40
roman-khimov
roman-khimov reviewed Jun 13, 2024
View reviewed changes
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/layer/multipart_upload.go
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

smallhive added 2 commits June 14, 2024 11:34
@smallhive
*: Replace split V1 to V2
72e8b8c 
Closes #937.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
layer: Fix internal tests
850d9d5 
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 <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from c699af0 to 2931301 Compare June 14, 2024 08:09
@smallhive smallhive requested a review from roman-khimov June 14, 2024 08:10
roman-khimov
roman-khimov reviewed Jun 14, 2024
View reviewed changes
smallhive added 5 commits June 14, 2024 15:58
@smallhive
*: Return correct info about owner
b69688c 
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 <evgenii@nspcc.io>
@smallhive
handler: Return real object size from header for encrypted objects.
cb41b6e 
Fixes s3tests_boto3/functional/test_s3.py::test_encryption_sse_c_multipart_bad_download test.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
workflow: Update component versions
aff34aa 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
neofs: Update error messages
b5f56f8 
Current texts are misleading a bit.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
*: Add zero part and use it as multipart uploadID
8328780 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 937-switch-to-v2-object-split-scheme branch from 2931301 to 8328780 Compare June 14, 2024 11:59
carpawell
carpawell approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@carpawell carpawell left a comment •
edited
Loading

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
@roman-khimov roman-khimov mentioned this pull request Nov 6, 2024
Closed
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 approved these changes

@carpawell carpawell carpawell 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.

GET of a multipart object fails Switch to v2 object split scheme
3 participants
@smallhive @roman-khimov @carpawell