Add retries to local sync download #9191
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Download Retry Fails, Progress Bars Leak
When retrying a failed download, the file is not truncated. The code only seeks to the beginning of the file, which can lead to file corruption if a previous attempt wrote partial data and the retry writes less, leaving leftover data. Additionally, a new progress bar is created for each retry attempt, resulting in multiple progress indicators for the same download and potential resource leaks, as previous bars are not cleaned up.
Was this report helpful? Give feedback by reacting with 👍 or 👎
itaigilo
added
include-changelog
N-o-Z
requested changes
Jun 18, 2025
There was a problem hiding this comment.
Some things on the higher level which I don't understand:
- SyncManager is already receiving a retryable http client, why do we need this mechanism as well?
- We already have a retry configuration parameter, why are we not using it?
- The PR states the changes should address issues with httpclient hangs and large files, however we catch all errors and retry on them. I'm not sure that's what we want.
- I did not understand the use case. There's no issue linked, no reproduction scenario or a sighted bug linked. Need more context
| } | ||
|
|
||
| // retryIfError attempts to execute the given operation up to maxAttempts times | ||
| func retryIfError(operation func(attempt int) error, maxAttempts int) error { |
There was a problem hiding this comment.
Suggested change
| func retryIfError(operation func(attempt int) error, maxAttempts int) error { | |
| func retryOnError(operation func(attempt int) error, maxAttempts int) error { |
Or just simply retry
| return nil | ||
| } | ||
|
|
||
| retriesCount := 3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Description
Background
Adding extra retries to
lakectl localsync (which is invoked by eithercloneorcheckout).These are an attempt to better handle socket hangouts of the storage backend, in extreme cases of very large files.
Testing Details
I haven't been able to reproduce the bug (which was reported by a client), but this addresses this specific reported failure.
Added unit-tests, and verified that a simple
local cloneandlocal checkoutwork as before.