rclone
stats: fix the speed not getting updated after a pause in the processing
What is the purpose of this change?
This shifts the behavior of the average loop to be a persistent loop that gets resumed/paused when transfers & checks are started/completed.
Previously, the averageLoop was stopped on completion of transfers & checks but failed to start again due to the protection of the sync.Once
Was the change discussed in an issue or in the forum before?
#7033
https://forum.rclone.org/t/speed-statistic-not-updating/46689
https://forum.rclone.org/t/rclone-copy-speed-statistic-not-updating/41860
Checklist
- [X] I have read the contribution guidelines.
- [X] I have added tests for all changes in this PR if appropriate.
- [X] I have added documentation for the changes if appropriate.
- [X] All commit messages are in house style.
- [X] I'm done, this Pull Request is ready for review :-)
Sure, I can give that a try.
However, I’m still not clear on exactly what the speed is supposed to represent on a mount. It can mean one of 2 things:
- The average speed of all files transferred across the lifetime of the mount.
- The average of the last set of transfers.
The latter seems to be the current approach based on the numbers, but both approaches make sense to me. It may also be that the calculation is not right when dealing with pausing/resuming the average loop.
Sure, I can give that a try.
:+1:
However, I’m still not clear on exactly what the speed is supposed to represent on a mount. It can mean one of 2 things:
- The average speed of all files transferred across the lifetime of the mount.
- The average of the last set of transfers.
The latter seems to be the current approach based on the numbers, but both approaches make sense to me. It may also be that the calculation is not right when dealing with pausing/resuming the average loop.
Hmm interesting point! It is certainly the second option now, but what will it be when the resuming works. Looking at the code I think it will be the average of the last set of transfers still.
average of the last set of transfers still
Yep, that's my understanding too and is probably not accurate for a mount that runs for weeks and has occasional bursts of transfers.
From my understanding, it won't exactly be the average of the last set of transfers, but the average during the last 16 periods of activity (due to the capping from the averagePeriod).
Yes... It is probably still useful as a measure of how fast the mount can transfer stuff - hopefully!
The tests are passing now, but I had to upgrade the RO lock to a RW lock, not sure if that matters here.
@ncw Let me know if you have any issues with this approach and will consider merging it if the conflicts are resolved. I don't want to go through the trouble of rebasing the fix just to let it go stale again and again get conflicts later.
Looks like we shouldn't be running the build and push docker stuff on PRs or branches!
Yeah, looks like the default Github Token doesn't allow package write permissions for pull requests. We could modify the workflow to use a more permissive token, but then there is nothing preventing a rogue PR (from a trusted account) from overwriting a tagged image.
References:
- https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
- https://docs.github.com/en/packages/managing-github-packages-using-github-actions-workflows/publishing-and-installing-a-package-with-github-actions#publishing-a-package-using-an-action
If it's fine with you, I will send a separate PR disabling the docker workflows for PR's.
Yeah, looks like the default Github Token doesn't allow package write permissions for pull requests. We could modify the workflow to use a more permissive token, but then there is nothing preventing a rogue PR (from a trusted account) from overwriting a tagged image.
Not good!
If it's fine with you, I will send a separate PR disabling the docker workflows for PR's.
Good idea.