download: dont cleanup directory after download failed #182
Conversation
MichaHoffmann
force-pushed
the
mhoffmann/dont-cleanup-objects-after-directory-download-failed
branch
3 times, most recently
from
8e46b87 to
62f0fe9
Compare
DownloadDir has the option to ignore paths that we have already on disk. In theory that makes downloads resumable, but in practice this doesnt work because we delete everything if one download fails. This means if one chunk in 1TiB block fails to download for whatever reason we start all over. Signed-off-by: Michael Hoffmann <mhoffmann@cloudflare.com>
MichaHoffmann
force-pushed
the
mhoffmann/dont-cleanup-objects-after-directory-download-failed
branch
from
62f0fe9 to
3bf609c
Compare
aknuds1
left a comment
aknuds1
left a comment
There was a problem hiding this comment.
Leaving some comments. I lack context to judge whether the change is going to break anything though. Let me ask the Mimir team.
| return downloadDir(ctx, logger, bkt, src, src, dst, options...) | ||
| } | ||
|
|
||
| func downloadDir(ctx context.Context, logger log.Logger, bkt BucketReader, originalSrc, src, dst string, options ...DownloadOption) error { |
There was a problem hiding this comment.
I'm wondering first of all whether downloadDir shouldn't return immediately if ctx.Err() is non-nil. The reason is that downloadDir can be called recursively with a context from errgroup.WithContext, that gets canceled when one or more of the goroutines fail.
| downloadedFiles = append(downloadedFiles, dst) | ||
| m.Unlock() | ||
| return nil | ||
| return DownloadFile(ctx, logger, bkt, name, dst) |
There was a problem hiding this comment.
Does DownloadFile respect cancellation via ctx? It ought to, so download can halt when one or more gouroutines fail.
| } | ||
|
|
||
| return nil | ||
| return g.Wait() |
There was a problem hiding this comment.
Why are we not calling g.Wait if err is non-nil? It seems wrong to me not to wait on the child gouroutines. I am also wondering whether a cancelable ctx should be created, that gets passed to errgroup.WithContext(ctx), and gets canceled when bkt.Iter returns an error. WDYT?
| testutil.Assert(t, isReaderAt) | ||
| } | ||
|
|
||
| func TestDownloadDir_CleanUp(t *testing.T) { |
There was a problem hiding this comment.
Should you keep this test, and assert that the downloaded files are kept?
|
The Mimir team's feedback is positive BTW (as already communicated in Slack) :) |
DownloadDir has the option to ignore paths that we have already on disk. In theory that makes downloads resumable, but in practice this doesnt work because we delete everything if one download fails. This means if one chunk in 1TiB block fails to download for whatever reason we start all over.
Changes
Verification