-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pga: safe downloads #69
Conversation
smola
commented
Jun 15, 2018
- pga: download files to temporary file first, fixes Make PGA downloader safe to cancel #39
- make get/list cancellable: Control+C will make pga to shutdown gracefully and clean up after itself, leaving no traces of temporary downloads
Note that I have not tested this with HDFS yet. |
Sounds awesome, going to start full download using this branch's |
High-level overview
😕 and seems like a pod has been re-started so there are no logs .... @smola number of files seems to be different, when using this branch
but number of .siva file in index is
|
So, the second run using this branch resulted in:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned with the way you're handling the errors from the deferred functions.
Seems a bit too "smart" for me.
PublicGitArchive/pga/cmd/cache.go
Outdated
@@ -97,3 +143,9 @@ func getIndex() (io.ReadCloser, error) { | |||
} | |||
return gzip.NewReader(f) | |||
} | |||
|
|||
func checkClose(name string, c io.Closer, err *error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this piece of code, tbh.
This is changing the value of the error in an almost magical way.
I understand why you're doing this, but I'd rather check have slightly more verbose code than this subtle interaction.
PublicGitArchive/pga/cmd/cache.go
Outdated
} | ||
|
||
func cancelableCopy(ctx context.Context, dst io.Writer, src io.Reader) ( | ||
int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put all in a line
PublicGitArchive/pga/cmd/cache.go
Outdated
default: | ||
} | ||
|
||
w, err := io.CopyN(dst, src, 512*1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 512*1024? should this be a named constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was an arbitrary number for testing. I'll change it.
@campoy Done. The |
PublicGitArchive/pga/cmd/cache.go
Outdated
const copyBufferSize = 512 * 1024 | ||
|
||
func cancelableCopy(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop blank line
|
||
const copyBufferSize = 512 * 1024 | ||
|
||
func cancelableCopy(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost tempted to propose this function to Go's stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great, but I doubt it gets accepted, since nothing in io
or io/ioutil
is cancellable. Also, there are prominent voices against that usage: Context isn’t for cancellation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small detail, but otherwise LGTM
if err := wc.Close(); err != nil { | ||
return fmt.Errorf("could not close %s: %v", dest.Abs(name), err) | ||
|
||
if err := rc.Close(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that failing to close a reader is not important, so it's ok to simply do defer rc.Close()
,
but I'll let you decide on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends on the reader implementation. Some of them signal unexpected EOF on Close
. You could argue that it is their fault to not use UnexpectedEOF
or some other errors on Read
though...
Signed-off-by: Santiago M. Mola <[email protected]>
Control+C will make pga to shutdown gracefully and clean up after itself, leaving no traces of temporary downloads. Signed-off-by: Santiago M. Mola <[email protected]>
Signed-off-by: Santiago M. Mola <[email protected]>
Merging, since we already verified that with this PR we avoid corrupt files. |