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

chunker: implement Resumer interface #5185

Closed
wants to merge 4 commits into from
Closed

chunker: implement Resumer interface #5185

wants to merge 4 commits into from

Conversation

mcalman
Copy link

@mcalman mcalman commented Mar 31, 2021 •
edited by ivandeex

What is the purpose of this change?

Implement the Resumer interface for the Chunker backend. Thus, making resumable uploads possible for Chunker.

Was the change discussed in an issue or in the forum before?

Fixes #5154
Based on pull request #4547 (add Resumer interface)
Related to feature request #87 (Resume uploads)
Related to thread https://forum.rclone.org/t/intelligent-faster-chunker-file-updates-on-checksum-enabled-remotes/22313/7

Orthogonal to feature request #5041 (multi-thread uploads in chunker)
Orthogonal to discussion #4798 (multi-thread uploads for different backends)

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

@ivandeex ivandeex marked this pull request as draft April 1, 2021 08:17
@Zstro

This comment has been minimized.

Sign in to view
@ivandeex

This comment has been minimized.

Sign in to view
@Zstro

This comment has been minimized.

Sign in to view
@ivandeex
Copy link
Member

ivandeex commented Apr 27, 2021 •
edited

@Zstro Your questions belong with discussion #4798 , you should subscribe to it.

@ivandeex

This comment has been minimized.

Sign in to view
@ivandeex ivandeex mentioned this pull request May 3, 2021
Open
5 tasks
@ivandeex ivandeex changed the title Chunker implement resumer chunker: implement Resumer interface May 3, 2021
@ivandeex ivandeex force-pushed the chunker-implement-resumer branch 2 times, most recently from 9a7e3db to 20a47e0 Compare May 3, 2021 12:36
@ivandeex ivandeex marked this pull request as ready for review May 3, 2021 12:37
@ivandeex

This comment has been minimized.

Sign in to view
@ivandeex ivandeex requested a review from ncw May 3, 2021 13:27
@ivandeex
Copy link
Member

ivandeex commented Jun 14, 2021 •
edited

@mcalman
I rebased this PR on top of current rclone master with added PR #4547.
Please don't force-push now.

The pre-state of commits was saved at ivandeex/mcalman-chunker-implement-resumer.

Please add relevant unit tests!

UPDATE 2021-07-23
@mcalman Please pull the branch from github and rebase it onto new #4547 (drop their old commit, keep their new commit and single commit from this pr, resolve conflicts if any). This will let you run unit tests, make an experimental build and run its beta-test in your environment, check how it feels from end-user perspective and prepare for review questions.

BTW, #4547 added a test where source does not change during interrupt. It would be good to also have a test where users attempts to resume from modified source, verify that resume machinery rejects it and think whether in result it feels convenient for end user.

@ivandeex ivandeex self-assigned this Jul 24, 2021
@ivandeex ivandeex added this to the v1.58 milestone Jul 24, 2021
@mcalman
Copy link
Author

mcalman commented Jul 27, 2021

BTW, #4547 added a test where source does not change during interrupt. It would be good to also have a test where users attempts to resume from modified source, verify that resume machinery rejects it and think whether in result it feels convenient for end user.

@ivandeex I added an adapted version of the test from #4547 as well as a new version of the test that uses a modified source. Currently, if a user has resuming enabled with --resume-larger and a modified source file is detected the resume is skipped for that file and the file is uploaded from scratch (a debug message also announces this).

@ivandeex
Copy link
Member

@ncw This one is ready. Please give it your applause review

@ivandeex ivandeex self-requested a review July 28, 2021 20:28
@ivandeex
Copy link
Member

rebased on fresh #4547 resolving a conflict after recent master merges
don't force-push

@mcalman
Copy link
Author

mcalman commented Aug 30, 2021

@ivandeex I noticed this PR is marked for v1.58 while the Resumer interface PR (#4547) is marked for v1.57. Pending @ncw 's review, do we think it may be possible to include both PRs in v1.57? This way there is some resume functionality for users to begin using.

@ivandeex
Copy link
Member

cc @ncw

@ivandeex
Copy link
Member

@Do we think it may be possible to include both PRs in v1.57?

I can't give a concrete answer in view of @ncw review delays increasing unpredictably.

This way there is some resume functionality for users to begin using.

I published a release on my repo fork with this goal in the mind. You could advertise your development on rclone forum to attract more volunteers. If you could demonstrate a convenient use case e.g. resuming a large download to local disk, it would bring more attention.

@ivandeex
Copy link
Member

ivandeex commented Sep 3, 2021

@ncw 🙏 🙏 🙏

ncw
ncw reviewed Sep 6, 2021
View reviewed changes
Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I'm happy for you to rebase and merge this after we've merged #4547 :-)

@ivandeex ivandeex added this to In progress in resume Sep 19, 2021
@ivandeex ivandeex removed their assignment Sep 30, 2021
@ivandeex ivandeex modified the milestones: v1.58, v1.57 Sep 30, 2021
@ivandeex
Copy link
Member

Rebased on top of current #4547

@ivandeex ivandeex mentioned this pull request Oct 16, 2021
Open
5 tasks
@ivandeex ivandeex self-assigned this Oct 18, 2021
@ivandeex ivandeex modified the milestones: v1.57, v1.58 Oct 18, 2021
@ivandeex ivandeex mentioned this pull request Oct 18, 2021
Closed
mcalman and others added 4 commits October 19, 2021 16:57
@mcalman @maxcalman
fs: add Resumer interface
b8fb9d1 
Added an interface for resuming failed uploads. Later on this interface
can be implemented by any backend where resumes are possible.

Fixes rclone#87

History of squashed commits:
- minor revisions made
- factored resume code to make Copy more readable
- limited cache cleaning to once per file uploaded
- finished refactoring
- added support for cache cleaning by total size
- improved cache cleaning
- moved cache cleaing to new function
- resume functions moved to seperate file
- added resume larger option
- added hashName and hashState to resume JSON
- minor fixes to error handling
- fixed consistency issue with fingerprints
- removed resume stub from chunker
- tweaked use of resume-larger option
- added local implementation and initial test
- fixed failing code quality tests
- used atexit for signal handling across OSs
- fixed for windows
- fix resume test after rebase
- fixed context convention violation and improved resume test
- increased test content size
- simulate larger reads in resume test
- fixed code quality checks
@mcalman @maxcalman
refactoring of cache directory naming and other minor changes
f820c66
@ivandeex @maxcalman
local/resume: improve atexit handler
bcc590a 
- report errors if any
- prevent double invocation
- prefer saved hash type when many are supported
@mcalman
chunker: implement resumer interface
a4b8504 
Fixes rclone#5154
@ivandeex
Copy link
Member

ivandeex commented Nov 17, 2021 •
edited

@mcalman
Unfortunately this PR is waiting for

which in turn is waiting for @ncw to review

which is spin off of

which is stuck forever.

I am sure we can solve it
I am going to untangle the knot when I have enough time for it,
say around next January
In the meantime I have added the patch from this PR on top of #4547
At any rate I will consider all parts together
I am closing this for now
cc @ncw

@ivandeex ivandeex closed this Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncw ncw ncw left review comments

@ivandeex ivandeex ivandeex approved these changes

Assignees

@ivandeex ivandeex

@mcalman mcalman

Labels
Remote: Chunker resume
Projects
resume
In progress
Milestone
v1.58
Development

Successfully merging this pull request may close these issues.

chunker: implement Resumer interface
4 participants
@mcalman @Zstro @ivandeex @ncw