[SPARK-19231][SPARKR] add error handling for download and untar for Spark release #16589
Conversation
|
Test build #71409 has finished for PR 16589 at commit
|
| @@ -54,7 +54,7 @@ | |||
| #' } | |||
| #' @param overwrite If \code{TRUE}, download and overwrite the existing tar file in localDir | |||
| #' and force re-install Spark (in case the local directory or file is corrupted) | |||
| #' @return \code{install.spark} returns the local directory where Spark is found or installed | |||
| #' @return the (invisible) local directory where Spark is found or installed | |||
There was a problem hiding this comment.
any reason to change the return value here ?
There was a problem hiding this comment.
it wasn't actually - it was always invisible - see here, just not documented as such. Also referencing the method name and saying returns again is redundant as this would go to the return section of the generated doc.
| @@ -201,14 +221,20 @@ directDownloadTar <- function(mirrorUrl, version, hadoopVersion, packageName, pa | |||
| msg <- sprintf(fmt, version, ifelse(hadoopVersion == "without", "Free build", hadoopVersion), | |||
| packageRemotePath) | |||
| message(msg) | |||
| downloadUrl(packageRemotePath, packageLocalPath, paste0("Fetch failed from ", mirrorUrl)) | |||
| downloadUrl(packageRemotePath, packageLocalPath) | |||
There was a problem hiding this comment.
Any reason to not use this Fetch failed from error message and instead use the error message from download.file
There was a problem hiding this comment.
There was a problem hiding this comment.
I didn't relate this to the update in L176 - I think this is fine. In general I think this file has gotten a little unwieldy with error messages coming from different functions. I wonder if there is a better way to refactor things to setup some expectations on where errors are thrown etc.
There was a problem hiding this comment.
yea I agree. I guess I'm trying to bubble up error messages to the top level but generally without exception to throw is making this non-trivial (never thought I'd say that!)
passing all the extra pieces of info around just to display them in error messages doesn't seem very intuitive either..
There was a problem hiding this comment.
Thanks @felixcheung - Change looks good to me in terms of catching the errors from untar. I think the overall file organization could be improved, but we can leave that to a future PR
| @@ -54,7 +54,7 @@ | |||
| #' } | |||
| #' @param overwrite If \code{TRUE}, download and overwrite the existing tar file in localDir | |||
| #' and force re-install Spark (in case the local directory or file is corrupted) | |||
| #' @return \code{install.spark} returns the local directory where Spark is found or installed | |||
| #' @return the (invisible) local directory where Spark is found or installed | |||
| @@ -201,14 +221,20 @@ directDownloadTar <- function(mirrorUrl, version, hadoopVersion, packageName, pa | |||
| msg <- sprintf(fmt, version, ifelse(hadoopVersion == "without", "Free build", hadoopVersion), | |||
| packageRemotePath) | |||
| message(msg) | |||
| downloadUrl(packageRemotePath, packageLocalPath, paste0("Fetch failed from ", mirrorUrl)) | |||
| downloadUrl(packageRemotePath, packageLocalPath) | |||
There was a problem hiding this comment.
I didn't relate this to the update in L176 - I think this is fine. In general I think this file has gotten a little unwieldy with error messages coming from different functions. I wonder if there is a better way to refactor things to setup some expectations on where errors are thrown etc.
…park release ## What changes were proposed in this pull request? When R is starting as a package and it needs to download the Spark release distribution we need to handle error for download and untar, and clean up, otherwise it will get stuck. ## How was this patch tested? manually Author: Felix Cheung <felixcheung_m@hotmail.com> Closes #16589 from felixcheung/rtarreturncode. (cherry picked from commit 278fa1e) Signed-off-by: Felix Cheung <felixcheung@apache.org>
|
I think its fine to leave it as is now - When we next add some feature or update the file we can do some refactoring |
…park release ## What changes were proposed in this pull request? When R is starting as a package and it needs to download the Spark release distribution we need to handle error for download and untar, and clean up, otherwise it will get stuck. ## How was this patch tested? manually Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#16589 from felixcheung/rtarreturncode.
…park release ## What changes were proposed in this pull request? When R is starting as a package and it needs to download the Spark release distribution we need to handle error for download and untar, and clean up, otherwise it will get stuck. ## How was this patch tested? manually Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#16589 from felixcheung/rtarreturncode.
What changes were proposed in this pull request?
When R is starting as a package and it needs to download the Spark release distribution we need to handle error for download and untar, and clean up, otherwise it will get stuck.
How was this patch tested?
manually