You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by bersprockets <gi...@git.apache.org> on 2018/01/04 04:53:14 UTC
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
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 <be...@...>
Date: 2018-01-03T22:32:30Z
Initial commit
commit 92e82d484a30a171141d2cdd1f800254fe33ceaf
Author: Bruce Robbins <be...@...>
Date: 2018-01-03T23:12:09Z
Remove test log messages
commit c5e835eb317d0b6970daef0ffc1214576874e8c6
Author: Bruce Robbins <be...@...>
Date: 2018-01-04T02:39:03Z
The 2 new methods should be private
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85718 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85718/testReport)** for PR 20147 at commit [`7b58d99`](https://github.com/apache/spark/commit/7b58d994f485255ffad59ed9ffb480a64a00cea2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/20147
OK to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85659 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85659/testReport)** for PR 20147 at commit [`c5e835e`](https://github.com/apache/spark/commit/c5e835eb317d0b6970daef0ffc1214576874e8c6).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85711 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85711/testReport)** for PR 20147 at commit [`3dbfffd`](https://github.com/apache/spark/commit/3dbfffd9a764cb35e05b85fc5c691a7708f31a0e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/20147
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85711 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85711/testReport)** for PR 20147 at commit [`3dbfffd`](https://github.com/apache/spark/commit/3dbfffd9a764cb35e05b85fc5c691a7708f31a0e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85717 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85717/testReport)** for PR 20147 at commit [`7b58d99`](https://github.com/apache/spark/commit/7b58d994f485255ffad59ed9ffb480a64a00cea2).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85717 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85717/testReport)** for PR 20147 at commit [`7b58d99`](https://github.com/apache/spark/commit/7b58d994f485255ffad59ed9ffb480a64a00cea2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85718 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85718/testReport)** for PR 20147 at commit [`7b58d99`](https://github.com/apache/spark/commit/7b58d994f485255ffad59ed9ffb480a64a00cea2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by bersprockets <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85659 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85659/testReport)** for PR 20147 at commit [`c5e835e`](https://github.com/apache/spark/commit/c5e835eb317d0b6970daef0ffc1214576874e8c6).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85689 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85689/testReport)** for PR 20147 at commit [`e85b813`](https://github.com/apache/spark/commit/e85b813889f5e66215e7298d045c132a5c6a7856).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85711/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85659/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20147
**[Test build #85689 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85689/testReport)** for PR 20147 at commit [`e85b813`](https://github.com/apache/spark/commit/e85b813889f5e66215e7298d045c132a5c6a7856).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85718/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/20147
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85689/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85717/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/20147
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by srowen <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSuite shou...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20147
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20147
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by cloud-fan <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20147: [SPARK-22940][SQL] HiveExternalCatalogVersionsSui...
Posted by HyukjinKwon <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org