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

Implement support for server chunked file upload V2 API #5939

Merged
merged 17 commits into from
Aug 31, 2023
Merged

Implement support for server chunked file upload V2 API #5939

merged 17 commits into from
Aug 31, 2023

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented Jul 31, 2023 •
edited
Loading

Implement #5819

@claucambra claucambra self-assigned this Jul 31, 2023
github-actions[bot]
github-actions bot previously requested changes Jul 31, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/libsync/propagateupload.h Outdated Show resolved Hide resolved
src/libsync/propagateupload.h Outdated Show resolved Hide resolved
src/libsync/syncoptions.h Outdated Show resolved Hide resolved
src/libsync/syncoptions.h Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 31, 2023 •
edited
Loading

Codecov Report

Merging #5939 (2f9dd79) into master (eec4069) will decrease coverage by 0.03%.
Report is 12 commits behind head on master.
The diff coverage is 90.90%.

❗ Current head 2f9dd79 differs from pull request most recent head a0c528a. Consider uploading reports for the commit a0c528a to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5939      +/-   ##
==========================================
- Coverage   60.09%   60.06%   -0.03%     
==========================================
  Files         145      145              
  Lines       18810    18838      +28     
==========================================
+ Hits        11303    11315      +12     
- Misses       7507     7523      +16
Files Changed Coverage Δ
src/common/syncjournaldb.h 75.00% <ø> (ø)
src/libsync/propagateupload.h 30.35% <0.00%> (ø)
src/libsync/propagateuploadv1.cpp 69.44% <33.33%> (ø)
src/common/syncjournaldb.cpp 79.53% <66.66%> (ø)
src/libsync/bulkpropagatorjob.cpp 74.71% <100.00%> (ø)
src/libsync/configfile.cpp 27.71% <100.00%> (+0.62%) ⬆️
src/libsync/owncloudpropagator.cpp 84.63% <100.00%> (ø)
src/libsync/propagateuploadng.cpp 84.72% <100.00%> (+2.33%) ⬆️
src/libsync/syncoptions.cpp 74.46% <100.00%> (+8.75%) ⬆️
src/libsync/syncoptions.h 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

@github-actions github-actions bot dismissed their stale review July 31, 2023 07:25

No clang-tidy warnings found so I assume my comments were addressed

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@tobiasKaminsky tobiasKaminsky linked an issue Aug 2, 2023 that may be closed by this pull request
Use MultipartUpload for uploading chunks to s3 #5819
Closed
@claucambra claucambra linked an issue Aug 13, 2023 that may be closed by this pull request
Support s3-multipart feature #5554
Closed
mgallien
mgallien requested changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

I am not done yet
see my comments and please fix the build failure in the drone CI pipeline

src/common/syncjournaldb.h Outdated Show resolved Hide resolved
src/libsync/bulkpropagatorjob.cpp Outdated Show resolved Hide resolved
@claucambra @mgallien
Const autofy startChunkUpload in propagatuploadng
795cd8c 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Refactor startChunkUpload, move upload finalisation to separate method
d99d35e 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Clean up declaration of propagateuploadng
12827d1 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Split getting chunk upload folder from chunkUrl as this was previousl…
614d48b 
…y misleading and unclear

Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Fix http error code uint warning
c29181e 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Conform to chunk sizing restrictions introduced in Chunking V2
cbbb4c8 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Start numbering chunks from 1 per chunking v2 api
fbb513a 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Sent destination header per chunking v2 api
a647ee1 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Assert that chunk num passed to chunkUrl is at least 1
a8a2a86 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Ensure chunkUploadFolder components are QStrings
7d90641 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Fix chunk numbering in chunking tests
189bc67 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Clarify use of ProgressInfo _chunk
7cb8640 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Enforce Chunk V2 API chunk size limits at the sync options level
2f5c164 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Add test for Chunk V2 restrictions in testchunkingng
eb01a57 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Use [[nodiscard]] for new syncoptions and propagateupload functions
eadb458 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Ensure default min and max chunk sizes are multiplied as long longs
f46d71f 
Signed-off-by: Claudio Cambra <[email protected]>
@claucambra @mgallien
Replace comments about relevancy of chunk to propagat v1 with variabl…
a0c528a 
…e name change

Signed-off-by: Claudio Cambra <[email protected]>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5939-a0c528a19044599eaad029b063f595697bb018aa-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

96.9% 96.9% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@claucambra claucambra merged commit ec9b940 into master Aug 31, 2023
8 of 10 checks passed
@claucambra claucambra deleted the feature/chunk-c2 branch August 31, 2023 14:45
@mgallien mgallien added this to the 3.10.0 milestone Sep 7, 2023
@solracsf solracsf mentioned this pull request Oct 27, 2023
Closed
@joshtrichards joshtrichards mentioned this pull request Aug 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgallien mgallien mgallien approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@camilasan camilasan Awaiting requested review from camilasan

@allexzander allexzander Awaiting requested review from allexzander

Assignees

@claucambra claucambra

Labels
None yet
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.

Use MultipartUpload for uploading chunks to s3 Support s3-multipart feature
3 participants
@claucambra @nextcloud-desktop-bot @mgallien