Conversation
| val protocol = getProtocol(marSavePath) | ||
|
|
||
| if ("file".equalsIgnoreCase(protocol)) { | ||
| print("Local") |
There was a problem hiding this comment.
remove or enhance these prints (or log)
There was a problem hiding this comment.
removed the debug statements
| jarFileList = jarFileList ::: List(new File(modelSrcDir.toString)) | ||
| } | ||
| else { | ||
| print("not local") |
| * @return The protocol for the given source. | ||
| */ | ||
| def getProtocol(source: String): String = { | ||
| require(source != null, "marfile source must not be null") |
| def getProtocol(source: String): String = { | ||
| require(source != null, "marfile source must not be null") | ||
|
|
||
| var protocol: String = null |
There was a problem hiding this comment.
make protocol a val and assign to the try statement directly
| catch { | ||
| case ex: Exception => | ||
| if (source.startsWith("//")) { | ||
| throw new IllegalArgumentException("Relative context: " + source) |
There was a problem hiding this comment.
"Relative context" - not sure how that message helps?
There was a problem hiding this comment.
changed it to: "Does not support Relative context: " + source
| result = file.toURI.toURL.getProtocol | ||
| } | ||
| catch { | ||
| case ex: Exception => result = "unknown" |
There was a problem hiding this comment.
I'm not a fan of this catch and return "unknown" --this mean you've go to have logic somewhere else to make sense of it. It's stronger to let the exception run free or at least have this method return a Try object.
| val protocol = ScoringModelUtils.getProtocol(storagePath) | ||
|
|
||
| if ("file".equalsIgnoreCase(protocol)) { | ||
| print("Local") |
There was a problem hiding this comment.
improve print message.
Does this really mean local? What about s3? --does it fall under local or hdsf? Could provide more help in making the right set of options, besides local and hdfs?
Also, this reveals to me that the protocol/path work you've added above is not unique to ScoringModelUtils, but more generally to sparktk saveload. That logic should move to this more generic location. What do you think?
2 participants
No description provided.