Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

MM-38701: Automatically retry on a failed export download #524

Merged
merged 9 commits into from
Jun 9, 2022

Conversation

agnivade
Copy link
Member

@agnivade agnivade commented Jun 8, 2022

We had a resume functionality which would download the file
again from a given offset. But clients would create wrapper
scripts which would auto-retry.

Therefore, we add that functionality in-built and deprecate
the resume flag. The reasoning behind removing the flag is that
if it doesn't succeed within 5 tries, then probably it's better
to attempt to download the file from scratch again sometime later.

https://mattermost.atlassian.net/browse/MM-38701

We had a resume functionality which would download the file
again from a given offset. But clients would create wrapper
scripts which would auto-retry.

Therefore, we add that functionality in-built and deprecate
the resume flag. The reasoning behind removing the flag is that
if it doesn't succeed within 5 tries, then probably it's better
to attempt to download the file from scratch again sometime later.

https://mattermost.atlassian.net/browse/MM-38701
@agnivade agnivade added the 2: Dev Review Requires review by a core committer label Jun 8, 2022
@agnivade agnivade requested review from isacikgoz and noxer June 8, 2022 09:02
@noxer
Copy link
Contributor

noxer commented Jun 8, 2022

Wouldn't it make sense to make the number of retries configurable. I can imaging people with an unstable internet connection wanting to retry it more often. Maybe even interpret a negative number as infinite retries.

@agnivade
Copy link
Member Author

agnivade commented Jun 8, 2022

That makes more sense. I'll make the changes.

@agnivade
Copy link
Member Author

agnivade commented Jun 8, 2022

@noxer - PTAL

@noxer
Copy link
Contributor

noxer commented Jun 8, 2022

Does the initial attempt count as a retry? It's a bit ambiguous but in this case --num_retries 0 should do one attempt and then give up. Other than that it looks good to me.

@agnivade
Copy link
Member Author

agnivade commented Jun 8, 2022

Heh good point, will update.

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

Looks good, have some styling comments below 👇

commands/export.go Outdated Show resolved Hide resolved
commands/export.go Outdated Show resolved Hide resolved
@agnivade agnivade requested a review from isacikgoz June 9, 2022 05:01
Copy link
Contributor

@noxer noxer left a comment

Choose a reason for hiding this comment

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

Code looks good. Thank you.

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

Nice!

@isacikgoz isacikgoz added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 9, 2022
@agnivade agnivade merged commit b7b23c0 into master Jun 9, 2022
@agnivade agnivade deleted the autoresume branch June 9, 2022 09:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants