[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-15 Thread asfgit 
Github user asfgit closed the pull request at:

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


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187943795
  
--- Diff: 
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -76,6 +75,7 @@ private[deploy] class SparkSubmitArguments(args: 
Seq[String], env: Map[String, S
   var proxyUser: String = null
   var principal: String = null
   var keytab: String = null
+  private var dynamicAllocationEnabled: String = null
--- End diff --

Wait, why not simply a boolean here? it's going to be much simpler to write.


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread jerryshao 
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187852287
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala ---
@@ -180,6 +180,25 @@ class SparkSubmitSuite
 appArgs.toString should include ("thequeue")
   }
 
+  test("SPARK-24241: not fail fast when executor num is 0 and dynamic 
allocation enabled") {
+val clArgs1 = Seq(
+  "--name", "myApp",
+  "--class", "Foo",
+  "--num-executors", "0",
+  "--conf", "spark.dynamicAllocation.enabled=true",
+  "thejar.jar")
+val appArgs = new SparkSubmitArguments(clArgs1)
+appArgs.dynamicAllocationEnabled should be ("true")
+
+val clArgs2 = Seq(
+  "--name", "myApp",
+  "--class", "Foo",
+  "--num-executors", "0",
+  "--conf", "spark.dynamicAllocation.enabled=false",
+  "thejar.jar")
+intercept[SparkException](new SparkSubmitArguments(clArgs2))
--- End diff --

Also please add a exception message check for `intercept`.


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread jerryshao 
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187852160
  
--- Diff: 
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -76,6 +75,7 @@ private[deploy] class SparkSubmitArguments(args: 
Seq[String], env: Map[String, S
   var proxyUser: String = null
   var principal: String = null
   var keytab: String = null
+  var dynamicAllocationEnabled: String = null
--- End diff --

Yes, I think you can remove that variable check in UT, seems not so 
necessary.


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread yaooqinn 
Github user yaooqinn commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187851318
  
--- Diff: 
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -76,6 +75,7 @@ private[deploy] class SparkSubmitArguments(args: 
Seq[String], env: Map[String, S
   var proxyUser: String = null
   var principal: String = null
   var keytab: String = null
+  var dynamicAllocationEnabled: String = null
--- End diff --

used in ut, so should i modify test or keep it `public`


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread jerryshao 
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187847736
  
--- Diff: 
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -76,6 +75,7 @@ private[deploy] class SparkSubmitArguments(args: 
Seq[String], env: Map[String, S
   var proxyUser: String = null
   var principal: String = null
   var keytab: String = null
+  var dynamicAllocationEnabled: String = null
--- End diff --

Add `private` since this variable is never used out of this class.


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-14 Thread jerryshao 
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/21290#discussion_r187847656
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala ---
@@ -180,6 +180,25 @@ class SparkSubmitSuite
 appArgs.toString should include ("thequeue")
   }
 
+  test("SPARK-24241: not fail fast when executor num is 0 and dynamic 
allocation enabled") {
--- End diff --

nit: "do not fail fast x when dynamic allocation is enabled" 


---

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

[GitHub] spark pull request #21290: [SPARK-24241][Submit]Do not fail fast when dynami...

2018-05-10 Thread yaooqinn 
GitHub user yaooqinn opened a pull request:

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

[SPARK-24241][Submit]Do not fail fast when dynamic resource allocation 
enabled with 0 executor

## What changes were proposed in this pull request?
```
~/spark-2.3.0-bin-hadoop2.7$ bin/spark-sql --num-executors 0 --conf 
spark.dynamicAllocation.enabled=true
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option PermSize=1024m; 
support was removed in 8.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option 
MaxPermSize=1024m; support was removed in 8.0
Error: Number of executors must be a positive number
Run with --help for usage help or --verbose for debug output
```

Actually, we could start up with min executor number with 0 before if 
dynamically

## How was this patch tested?

ut added


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

$ git pull https://github.com/yaooqinn/spark SPARK-24241

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

https://github.com/apache/spark/pull/21290.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 #21290


commit 844574bb4ce3e11b09d8bf52f4b15f52398e2605
Author: Kent Yao 
Date:   2018-05-10T08:59:46Z

[SPARK-24241][Submit]Do not fail fast when dynamic resource allocation 
enabled with 0 executor




---

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