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

Remove file from bulk upload black list only when upload succeeded. #5998

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove file from bulk upload black list only when upload succeeded. #5998

wants to merge 2 commits into from
+119 −9

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented Aug 22, 2023 •
edited
Loading

*) first try to upload a batch with the current size
*) 504 errors would divide the current batch size by a factor of 2
*) all errors still put the small files in the blacklist for bulk upload

  • On error put item on blacklist
    • Schedule another sync with smaller batch
    • Do another discovery (?)
    • if sync succeeds, remove from the blacklist
    • if error again, cancel sync

@codecov
Copy link

codecov bot commented Aug 22, 2023 •
edited
Loading

Codecov Report

Merging #5998 (834f70f) into master (3dc583c) will decrease coverage by 0.67%.
Report is 166 commits behind head on master.
The diff coverage is 40.00%.

❗ Current head 834f70f differs from pull request most recent head 76ddabe. Consider uploading reports for the commit 76ddabe to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5998      +/-   ##
==========================================
- Coverage   60.79%   60.12%   -0.67%     
==========================================
  Files         145      145              
  Lines       18836    18884      +48     
==========================================
- Hits        11451    11354      -97     
- Misses       7385     7530     +145
Files Coverage Δ
src/libsync/bulkpropagatorjob.h 50.00% <ø> (ø)
src/libsync/logger.cpp 33.33% <ø> (-14.27%) ⬇️
src/libsync/owncloudpropagator.cpp 84.50% <100.00%> (-0.02%) ⬇️
src/libsync/owncloudpropagator.h 65.35% <100.00%> (ø)
src/libsync/bulkpropagatorjob.cpp 72.62% <33.33%> (-2.10%) ⬇️

... and 30 files with indirect coverage changes

@camilasan camilasan force-pushed the bugfix/bulk-upload branch 3 times, most recently from d17bc73 to 08f7fc2 Compare September 12, 2023 13:46
@camilasan camilasan marked this pull request as ready for review October 10, 2023 15:20
mgallien
mgallien requested changes Oct 11, 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.

can you try adding an automated test for the new behavior ?

src/libsync/bulkpropagatorjob.cpp Show resolved Hide resolved
src/libsync/bulkpropagatorjob.cpp Show resolved Hide resolved
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

26.3% 26.3% Coverage
0.0% 0.0% Duplication

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

@camilasan camilasan force-pushed the bugfix/bulk-upload branch 3 times, most recently from ccf2462 to 76ddabe Compare January 30, 2024 10:15
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5998-76ddabe08cbf0c5d7f79c1d94db1cefd102faeda-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.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

12 New Code Smells (required ≤ 0)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

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

@Rello Rello added this to the 3.16.0 milestone Dec 13, 2024
Camila and others added 2 commits February 5, 2025 16:06
@camilasan
Change bulk upload batch size when there is a sync error.
0a2498b 
- Cancel sync after try with smaller bulk upload batch size also throws error.
- Continue syncing when there is no error.
- Remove file from bulk upload black list only when upload succeeded.

Signed-off-by: Camila <[email protected]>
@camilasan
Add test for syncing with bulk upload when there are errors.
f03391b 
Signed-off-by: Camila Ayres <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2025

Artifact containing the AppImage: nextcloud-appimage-pr-5998.zip

SHA256 checksum: 374364a05a91760203c8b3c1dba06570015918e32b28059ad0c645ce79cd9cb1

To test this change/fix you can download the above artifact file, unzip it, and run it.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgallien mgallien mgallien requested changes

@claucambra claucambra claucambra approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@camilasan camilasan

Labels
None yet
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@camilasan @nextcloud-desktop-bot @mgallien @claucambra @Rello