Skip to content
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@

package org.apache.spark.sql.hive

import java.io.File
import java.io.{File, FileInputStream}
import java.nio.file.Files

import scala.io.Codec
import scala.io.Source
import scala.sys.process._

import org.apache.spark.TestUtils
import org.apache.hadoop.conf.Configuration

import org.apache.spark.{SecurityManager, SparkConf, TestUtils}
import org.apache.spark.sql.{QueryTest, Row, SparkSession}
import org.apache.spark.sql.catalyst.TableIdentifier
import org.apache.spark.sql.catalyst.catalog.CatalogTableType
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use filename in interpolation also?

logInfo(s"Downloading Spark $version from $url")
if (Seq("wget", url, "-q", "-P", path).! == 0) {
if (getFileFromUrl(url, path, filename)) {
return
}
logWarning(s"Failed to download Spark $version from $url")
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result.exists()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch {
case ex: Exception => logError("Could not get file from url " + urlString + ": "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+ ex.getMessage)
false
}
}

private def getStringFromUrl(urlString: String, encoding: String = "UTF-8"): String = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I should have caught that. Will fix.

val outDir = Files.createTempDirectory("string-")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

val filename = "string-out.txt"

if (!getFileFromUrl(urlString, outDir.toString, filename)) {
throw new java.io.IOException("Could not get string from url " + urlString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import this

}

val outputFile = new File(outDir.toString + File.separator + filename)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val fis = new FileInputStream(outputFile)
val result = Source.fromInputStream(fis)(Codec(encoding)).mkString
fis.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close() in a finally block?

outputFile.delete()
outDir.toFile.delete()
result
}

override def beforeAll(): Unit = {
super.beforeAll()

Expand Down