[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/20147


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159939270
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +93,34 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Unit = {
+val conf = new SparkConf
+// if the caller passes the name of an existing file, we want 
doFetchFile to write over it with
+// the contents from the specified url.
+conf.set("spark.files.overwrite", "true")
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+// propagate exceptions up to the caller of getFileFromUrl
--- End diff --

We generally don't add these kind of comments since it's implied in every 
statement outside of a try...catch.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[bersprockets]

Github user bersprockets commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159802199
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +93,34 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Unit = {
+val conf = new SparkConf
+// if the caller passes the name of an existing file, we want 
doFetchFile to write over it with
+// the contents from the specified url.
+conf.set("spark.files.overwrite", "true")
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+// propagate exceptions up to the caller of getFileFromUrl
+Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, 
hadoopConf)
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
--- End diff --

Oops. I should have caught that. Will fix.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[HyukjinKwon]

Github user HyukjinKwon commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159798210
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +93,34 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Unit = {
+val conf = new SparkConf
+// if the caller passes the name of an existing file, we want 
doFetchFile to write over it with
+// the contents from the specified url.
+conf.set("spark.files.overwrite", "true")
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+// propagate exceptions up to the caller of getFileFromUrl
+Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, 
hadoopConf)
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
--- End diff --

Seems `encoding: String = "UTF-8"` is not used.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159757298
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new IOException("Could not get string from url " + urlString)
--- End diff --

How about letting the exception from `doFetchFile` propagate, and only 
handle it as part of the retries in `tryDownloadSpark`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159755262
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
--- End diff --

Use `Utils.createTempDir`; it handles deleting the directory for you later.
 
Another option is to use `File.createTempFile` + `File.deleteOnExit`. Same 
result.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159754446
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -56,10 +60,11 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 // Try mirrors a few times until one succeeds
 for (i <- 0 until 3) {
   val preferredMirror =
-Seq("wget", 
"https://www.apache.org/dyn/closer.lua?preferred=true;, "-q", "-O", "-").!!.trim
-  val url = 
s"$preferredMirror/spark/spark-$version/spark-$version-bin-hadoop2.7.tgz"
+
getStringFromUrl("https://www.apache.org/dyn/closer.lua?preferred=true;)
+  val filename = s"spark-$version-bin-hadoop2.7.tgz"
+  val url = s"$preferredMirror/spark/spark-$version/" + filename
--- End diff --

Use `filename` in interpolation also?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159756262
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new IOException("Could not get string from url " + urlString)
+}
+
+val contentFile = new File(outDir.toFile, filename)
+try {
+  Source.fromFile(contentFile)(Codec(encoding)).mkString
+} finally {
+  contentFile.delete()
--- End diff --

You can get rid of this finally block if you switch to 
`Utils.createTempDir`.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159756731
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
--- End diff --

In case of multi-line case statements, put them in the next line, properly 
indented.
 
Also: `logWarning(s"I shall prefer interpolation", ex)`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159754913
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
--- End diff --

Use `Charset` instead of String... but since you're really not using that 
parameter, I'd just hardcode the charset in the only place where it's used.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159755811
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logWarning("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new IOException("Could not get string from url " + urlString)
+}
+
+val contentFile = new File(outDir.toFile, filename)
+try {
+  Source.fromFile(contentFile)(Codec(encoding)).mkString
--- End diff --

This is ok, but there's an easier / cleaner API for this. Look around for 
calls like this:

Files.toString(file, StandardCharsets.UTF_8);

  


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159756988
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
--- End diff --

This seems unnecessary (just return `true`? `doFetchFile` should always 
return a file that exists), but ok.
  


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159665608
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
--- End diff --

Meh, if we can reuse code instead of rewriting it, seems OK to me


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159665269
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logError("Could not get file from url " + 
urlString + ": "
--- End diff --

This should just be a warning (it's not fatal) and you could use string 
interpolation.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159665546
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logError("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new java.io.IOException("Could not get string from url " + 
urlString)
+}
+
+val outputFile = new File(outDir.toString + File.separator + filename)
+val fis = new FileInputStream(outputFile)
+val result = Source.fromInputStream(fis)(Codec(encoding)).mkString
+fis.close()
--- End diff --

close() in a finally block?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159665346
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logError("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new java.io.IOException("Could not get string from url " + 
urlString)
--- End diff --

Import this


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159665512
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
+  result.exists()
+} catch {
+  case ex: Exception => logError("Could not get file from url " + 
urlString + ": "
++ ex.getMessage)
+false
+}
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = 
"UTF-8"): String = {
+val outDir = Files.createTempDirectory("string-")
+val filename = "string-out.txt"
+
+if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+  throw new java.io.IOException("Could not get string from url " + 
urlString)
+}
+
+val outputFile = new File(outDir.toString + File.separator + filename)
--- End diff --

I think you want to use the constructor that takes File, String rather than 
construct the path. MInor thing.
I'd also say use the newer Path API but may not be what Scala APIs use.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20147#discussion_r159582192
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala
 ---
@@ -85,6 +90,43 @@ class HiveExternalCatalogVersionsSuite extends 
SparkSubmitTestUtils {
 new File(tmpDataDir, name).getCanonicalPath
   }
 
+  private def getFileFromUrl(urlString: String, targetDir: String, 
filename: String): Boolean = {
+val conf = new SparkConf
+val securityManager = new SecurityManager(conf)
+val hadoopConf = new Configuration
+
+val outDir = new File(targetDir)
+if (!outDir.exists()) {
+  outDir.mkdirs()
+}
+
+try {
+  val result = Utils.doFetchFile(urlString, outDir, filename, conf, 
securityManager, hadoopConf)
--- End diff --

`Utils.doFetchFile` is a little overkill for this case, maybe we can just 
implement a simple downloading function with java.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...

[bersprockets]

GitHub user bersprockets opened a pull request:

https://github.com/apache/spark/pull/20147

[SPARK-22940][SQL] HiveExternalCatalogVersionsSuite should succeed on 
platforms that don't have wget

## What changes were proposed in this pull request?

Modified HiveExternalCatalogVersionsSuite.scala to use Utils.doFetchFile to 
download different versions of Spark binaries rather than launching wget as an 
external process.

On platforms that don't have wget installed, this suite fails with an error.

@cloud-fan : would you like to check this change?

## How was this patch tested?

1) test-only of HiveExternalCatalogVersionsSuite on several platforms. 
Tested bad mirror, read timeout, and redirects.
2) ./dev/run-tests



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bersprockets/spark SPARK-22940-alt

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20147.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20147


commit 5fe679f3271692729e8b9a2a9351725a80fcf8f6
Author: Bruce Robbins 
Date:   2018-01-03T22:32:30Z

Initial commit

commit 92e82d484a30a171141d2cdd1f800254fe33ceaf
Author: Bruce Robbins 
Date:   2018-01-03T23:12:09Z

Remove test log messages

commit c5e835eb317d0b6970daef0ffc1214576874e8c6
Author: Bruce Robbins 
Date:   2018-01-04T02:39:03Z

The 2 new methods should be private




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org