lakectl enhance downloader reduce call to stat object for faster download #9555
Conversation
nopcoder
added
area/lakectl
cmd/lakectl/cmd/fs_download.go
Outdated
|
|
||
| type objectInfo struct { | ||
| relPath string | ||
| object *apigen.ObjectStats |
There was a problem hiding this comment.
| object *apigen.ObjectStats | |
| stats *apigen.ObjectStats |
There was a problem hiding this comment.
updated to objectStat
cmd/lakectl/cmd/fs_download.go
Outdated
| ) | ||
|
|
||
| type objectInfo struct { | ||
| relPath string |
There was a problem hiding this comment.
relative to what? document
There was a problem hiding this comment.
added comment.
relative to the source path in the repo - used the same terminology used on lakectl fs download code.
| // download object | ||
| var err error | ||
| if d.PreSign && swag.Int64Value(objectStat.SizeBytes) >= d.PartSize { | ||
| // download using presigned multipart download, it will fall back to presign single object download if needed |
There was a problem hiding this comment.
When will it fallback to presign single object download?
There was a problem hiding this comment.
Cloud return an error in case the object size < d.PartSize, but instead it will just call downloadObject.
I've added a check here, before the call just not to go through the fallback.
Both are internal methods - I cloud just return an error in downloadPresignMultipart, wdyt?
There was a problem hiding this comment.
If downloadPresignMultipart is only called from here, you can remove the internal check in downloadPresignMultipart. If it's not, then leave it as is
| // download object | ||
| var err error | ||
| if d.PreSign && swag.Int64Value(objectStat.SizeBytes) >= d.PartSize { | ||
| // download using presigned multipart download, it will fall back to presign single object download if needed |
There was a problem hiding this comment.
If downloadPresignMultipart is only called from here, you can remove the internal check in downloadPresignMultipart. If it's not, then leave it as is
|
@itaiad200 refactor the code for more reuse and single place to check the preconditions for download without object stat. |
| // avoiding the need for a separate stat call. | ||
| func (d *Downloader) DownloadWithObjectInfo(ctx context.Context, src uri.URI, dst string, tracker *progress.Tracker, objectStat *apigen.ObjectStats) error { | ||
| // downloadObjectCore handles the common download logic for both Download and DownloadWithObjectInfo | ||
| func (d *Downloader) downloadObjectCore(ctx context.Context, src uri.URI, dst string, tracker *progress.Tracker, objectStat *apigen.ObjectStats) error { |
There was a problem hiding this comment.
Is this better?
| func (d *Downloader) downloadObjectCore(ctx context.Context, src uri.URI, dst string, tracker *progress.Tracker, objectStat *apigen.ObjectStats) error { | |
| func (d *Downloader) downloadWithObjectInfo(ctx context.Context, src uri.URI, dst string, tracker *progress.Tracker, objectStat *apigen.ObjectStats) error { |
There was a problem hiding this comment.
didn't want to mix with DownloadWithObjectInfo
| if size < d.PartSize { | ||
| return d.downloadObject(ctx, src, dst, tracker) | ||
| return fmt.Errorf("object is smaller than PartSize (%d): %w", d.PartSize, ErrValidation) | ||
| } |
There was a problem hiding this comment.
This code path is unreachable
There was a problem hiding this comment.
this is just to be on the safe side if someone will call this function without checking the minimum size.
2 participants
use object listing stat information to remove extra call for stat during lakectl fs download.
kept call to stat object when we require presign url for multipart download of large file.
Close #9557