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

bersprockets · 2018-01-04T04:53:13Z

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?

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

cloud-fan · 2018-01-04T05:04:46Z

OK to test

HyukjinKwon · 2018-01-04T05:12:02Z

ok to test

cloud-fan · 2018-01-04T05:16:21Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    }
+
+    try {
+      val result = Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)


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

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

SparkQA · 2018-01-04T07:07:28Z

Test build #85659 has finished for PR 20147 at commit c5e835e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-01-04T14:33:33Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+      val result = Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)
+      result.exists()
+    } catch {
+      case ex: Exception => logError("Could not get file from url " + urlString + ": "


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

srowen · 2018-01-04T14:33:50Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    val filename = "string-out.txt"
+
+    if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+      throw new java.io.IOException("Could not get string from url " + urlString)


Import this

srowen · 2018-01-04T14:34:39Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+      throw new java.io.IOException("Could not get string from url " + urlString)
+    }
+
+    val outputFile = new File(outDir.toString + File.separator + filename)


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.

srowen · 2018-01-04T14:34:49Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    val outputFile = new File(outDir.toString + File.separator + filename)
+    val fis = new FileInputStream(outputFile)
+    val result = Source.fromInputStream(fis)(Codec(encoding)).mkString
+    fis.close()


close() in a finally block?

srowen · 2018-01-04T14:35:07Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    }
+
+    try {
+      val result = Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)


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

SparkQA · 2018-01-04T20:57:41Z

Test build #85689 has finished for PR 20147 at commit e85b813.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-01-04T21:02:48Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

-      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


Use filename in interpolation also?

vanzin · 2018-01-04T21:05:20Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    }
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = "UTF-8"): String = {


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.

vanzin · 2018-01-04T21:07:02Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = "UTF-8"): String = {
+    val outDir = Files.createTempDirectory("string-")


Use Utils.createTempDir; it handles deleting the directory for you later.

Another option is to use File.createTempFile + File.deleteOnExit. Same result.

vanzin · 2018-01-04T21:09:13Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+
+    val contentFile = new File(outDir.toFile, filename)
+    try {
+      Source.fromFile(contentFile)(Codec(encoding)).mkString


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

Files.toString(file, StandardCharsets.UTF_8);

vanzin · 2018-01-04T21:11:35Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    try {
+      Source.fromFile(contentFile)(Codec(encoding)).mkString
+    } finally {
+      contentFile.delete()


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

vanzin · 2018-01-04T21:13:54Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+      val result = Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)
+      result.exists()
+    } catch {
+      case ex: Exception => logWarning("Could not get file from url " + urlString + ": "


In case of multi-line case statements, put them in the next line, properly indented.

Also: logWarning(s"I shall prefer interpolation", ex)

vanzin · 2018-01-04T21:15:06Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+
+    try {
+      val result = Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)
+      result.exists()


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

vanzin · 2018-01-04T21:16:41Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    val filename = "string-out.txt"
+
+    if (!getFileFromUrl(urlString, outDir.toString, filename)) {
+      throw new IOException("Could not get string from url " + urlString)


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

HyukjinKwon · 2018-01-05T02:02:34Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+    Utils.doFetchFile(urlString, outDir, filename, conf, securityManager, hadoopConf)
+  }
+
+  private def getStringFromUrl(urlString: String, encoding: String = "UTF-8"): String = {


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

Oops. I should have caught that. Will fix.

SparkQA · 2018-01-05T03:58:08Z

Test build #85711 has finished for PR 20147 at commit 3dbfffd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-05T05:43:48Z

Test build #85717 has finished for PR 20147 at commit 7b58d99.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-05T05:51:27Z

retest this please

SparkQA · 2018-01-05T07:53:50Z

Test build #85718 has finished for PR 20147 at commit 7b58d99.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin

LGTM. Merging to master / 2.3.

Left a comment but no need to address it.

vanzin · 2018-01-05T17:48:33Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala

+      outDir.mkdirs()
+    }
+
+    // propagate exceptions up to the caller of getFileFromUrl


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

… 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 Author: Bruce Robbins <bersprockets@gmail.com> Closes #20147 from bersprockets/SPARK-22940-alt. (cherry picked from commit c0b7424) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

bersprockets added 3 commits January 3, 2018 14:32

Initial commit

5fe679f

Remove test log messages

92e82d4

The 2 new methods should be private

c5e835e

cloud-fan reviewed Jan 4, 2018

View reviewed changes

srowen requested changes Jan 4, 2018

View reviewed changes

bersprockets added 2 commits January 4, 2018 10:32

Address pull request comments

8b71ea3

Remove testing log statements

e85b813

vanzin reviewed Jan 4, 2018

View reviewed changes

bersprockets added 3 commits January 4, 2018 16:39

Address pull request comments

51f3589

Remove test log messages

46225e8

Fix comment typo

3dbfffd

HyukjinKwon reviewed Jan 5, 2018

View reviewed changes

Remove unused parameter in getStringFromUrl signature

7b58d99

vanzin reviewed Jan 5, 2018

View reviewed changes

asfgit closed this in c0b7424 Jan 5, 2018

bersprockets deleted the SPARK-22940-alt branch March 5, 2018 23:05

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

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

Uh oh!

Conversation

bersprockets commented Jan 4, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Jan 4, 2018

Uh oh!

HyukjinKwon commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 5, 2018

Uh oh!

SparkQA commented Jan 5, 2018

Uh oh!

HyukjinKwon commented Jan 5, 2018

Uh oh!

SparkQA commented Jan 5, 2018

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!