Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2848#issuecomment-65737090
Hi @ryan-williams,
I took a quick glance through the code and find the new `maybeMoveFile
`method to be a bit confusing:
- It takes a `url` parameter that it doesn't use for anything other than
logging.
- The name `maybe` implies an operation that can succeed or fail; it would
be nice if the method's documentation clarified how success / failure is
reported, maybe as a Scaladoc `@throws` annotation of a `Try` or something.
This is important since the caller may need to know that the source file wasn't
moved so that it can perform proper cleanup (e.g. delete the file).
- The different cases don't seem to be exhaustively handled; what happens
if the destination file _doesn't_ already exist?
- One of the overloaded versions seems like it has two responsibilities:
download a file then move it; it would be nice to separate these concerns to
keep the individual methods simple and easy to understand.
Do you know if we have a test suite for this file copying code? If not, we
should create one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
