from:"GraceH"

[GitHub] spark issue #7927: [SPARK-9591][CORE]Job may fail for exception during getti...

2016-08-22 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/7927
  
@sprite331. According to my understanding, this patch tries to catch 
certain exceptions when the user introducing dynamic allocation. One quick 
solution is to disable dynamic allocation if possible, which can avoid certain 
exception (negative part is to miss that new function introduced since 1.3). 
Another one, you can try to catch that exception by yourself (if you upgrade 
your 1.3 deployments).  I am not sure if either solutions work to you or not. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Document additional options in jdbc Writer

2016-08-22 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
Thanks @srowen 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Document additional options in jdbc Writer

2016-08-21 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
@srowen, I have revised that accordingly. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Document additional options in jdbc Writer

2016-08-19 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
@srowen. I have updated the patch accordingly. please let me know your 
comments. anything missing, please let me know.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-18 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
Sorry about my mistake. I will re-post one.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-18 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
Oops. @srowen I thought the previous pull request to be closed without 
merge. That is why I re-post that here. 
Do you mean we just need the document here, right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14683: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-17 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14683
  
@srowen Here we go. please feel free to let me know your comments.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14683: [SPARK-16968]Add additional options in jdbc when ...

2016-08-17 Thread GraceH

GitHub user GraceH opened a pull request:

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

[SPARK-16968]Add additional options in jdbc when creating a new table

## What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)
In the PR, we just allow the user to add additional options when create a 
new table in JDBC writer. 
The options can be table_options or partition_options.
E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT CHARSET=utf8"

Here is the usage example:
```
df.write.option("createTableOptions", "ENGINE=InnoDB DEFAULT 
CHARSET=utf8").jdbc(...)
```

## How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration 
tests, manual tests)
Unit test has been added. 

(If this patch involves UI changes, please attach a screenshot; otherwise, 
remove this)



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

$ git pull https://github.com/GraceH/spark jdbc_options

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

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


commit b302b1c7ec75ae1e78d132f7ecdb9bb7f33816d4
Author: GraceH <93113...@qq.com>
Date:   2016-08-09T06:47:51Z

Add additional options in jdbc when creating a new table

commit 6a3cb4226027e7d22b7606c0e890d258eb8da138
Author: GraceH <93113...@qq.com>
Date:   2016-08-09T09:57:36Z

organize the code with better format and rename the option name

commit eb0656b1b0723e7e7d2fe9f3f9b0ca339772076f
Author: GraceH <93113...@qq.com>
Date:   2016-08-09T10:09:37Z

fix code style issue

commit 57be055c542d1720bb9fd57810d4c2593444
Author: GraceH <93113...@qq.com>
Date:   2016-08-11T05:28:06Z

merge jdbc writer options to JDBCOptions

commit 4fb5e55a50531abf255169c275ad2ad2cf2d71f2
Author: GraceH <93113...@qq.com>
Date:   2016-08-12T04:57:19Z

add the unit test for JDBCWriter with createTableOptions

commit 186a5828fddfdd9c6e2d778ff28b30d3b1d2c471
Author: GraceH <93113...@qq.com>
Date:   2016-08-12T05:08:50Z

fix scala style issue

commit d0bdd35acfe8181612efdb22ac96280af70b354e
Author: GraceH <93113...@qq.com>
Date:   2016-08-12T05:19:46Z

fix scala style issue with redundant spaces

commit 21b4278c2e6d46510645464ac083d83108693133
Author: GraceH <93113...@qq.com>
Date:   2016-08-12T05:31:33Z

    remove private[sql]

commit 5176fdb90e6ea0bc2b7cc4e1e5d36811d8403b93
Author: GraceH <93113...@qq.com>
Date:   2016-08-12T07:23:34Z

change url and table to jdbcOptions' member var

commit 8360c2911b70aa628f8edba593e3764d3b07ca55
Author: Jie Huang <jhua...@paypal.com>
Date:   2016-08-17T10:00:46Z

Document JDBC Writer options




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14559: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-17 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14559
  
Hi @srowen @rxin , sorry for late response. I have added the document part. 
https://github.com/GraceH/spark/commit/8360c2911b70aa628f8edba593e3764d3b07ca55
Shall I raise a new PR?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14559: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-14 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14559
  
sure. Both are ok to me. will document those options. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-12 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74550111
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -423,6 +423,10 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
 props.putAll(connectionProperties)
 val conn = JdbcUtils.createConnectionFactory(url, props)()
 
+// to add required options like URL and dbtable
+val params = extraOptions.toMap ++ Map("url" -> url, "dbtable" -> 
table)
+val jdbcOptions = new JDBCOptions(params)
+
 try {
   var tableExists = JdbcUtils.tableExists(conn, url, table)
--- End diff --

BTWï¼ there are several places of (url and table). shall we replace all of 
them?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-12 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74546716
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -423,6 +423,10 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
 props.putAll(connectionProperties)
 val conn = JdbcUtils.createConnectionFactory(url, props)()
 
+// to add required options like URL and dbtable
+val params = extraOptions.toMap ++ Map("url" -> url, "dbtable" -> 
table)
+val jdbcOptions = new JDBCOptions(params)
+
 try {
   var tableExists = JdbcUtils.tableExists(conn, url, table)
--- End diff --

Sure. make sense.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-11 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74542628
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala
 ---
@@ -20,14 +20,21 @@ package org.apache.spark.sql.execution.datasources.jdbc
 /**
  * Options for the JDBC data source.
  */
-private[jdbc] class JDBCOptions(
+private[sql] class JDBCOptions(
--- End diff --

OK. Just intend to follow that origin style. I will fix that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14559: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-11 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14559
  
Thanks all. I have added the unit test in JDBCWriterSuite. Any further 
comment, please feel free to let me know. 

BTW, or we can point the user to check JDBCOptions for further 
configuration information.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #14559: [SPARK-16968]Add additional options in jdbc when creatin...

2016-08-10 Thread GraceH

Github user GraceH commented on the issue:

https://github.com/apache/spark/pull/14559
  
@HyukjinKwon and @srowen, here is the initial proposal. Please let me know 
your comment. I will refine that with unit test later.

BTW, the readwriter.py calls high level api of jdbc(url, table, 
connectionProperties). If we don't change that API like reader api does, we may 
not need to expose the JDBCOptions in that file. What do you think? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-10 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74278231
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,11 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = 
extraOptions.getOrElse("createTableOptions", "")
--- End diff --

@HyukjinKwon  For those database specific options can be merged as 
createTableOptions. Not recommend to enumerate them one by one. Just as @srowen 
suggested.

Thanks to @HyukjinKwon and @srowen. I will propose a draft, and back to 
both of you later. Thanks. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74188842
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,11 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = 
extraOptions.getOrElse("createTableOptions", "")
--- End diff --

A quick question here. The JDBCOptions also contain URL and table 
information, like:
```
 // a JDBC URL
  val url = parameters.getOrElse("url", sys.error("Option 'url' not 
specified"))
  // name of table
  val table = parameters.getOrElse("dbtable", sys.error("Option 'dbtable' 
not specified"))
```
Shall we merge those information into current DataFrameWriter as well?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74187289
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,11 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = 
extraOptions.getOrElse("createTableOptions", "")
--- End diff --

OK. Let me take a look. and give a quick version.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74029250
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,16 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = {
+  extraOptions.get("jdbc.create.table.options") match {
+case Some(value) => " " + value
+case None => ""
+  }
+}
+val sql = s"CREATE TABLE $table ($schema)" + createtblOptions
--- End diff --

Totally got your idea. Thanks a lot for the prompt response. I will update 
the patch. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74028584
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,16 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = {
+  extraOptions.get("jdbc.create.table.options") match {
+case Some(value) => " " + value
+case None => ""
+  }
+}
+val sql = s"CREATE TABLE $table ($schema)" + createtblOptions
--- End diff --

To have a better format(to tell two parts explicitly), how about this?
```
val createtblOptions = extraOptions.getOrElse("createTableOptions", "")
val sql = s"CREATE TABLE $table ($schema) $createtblOptions"
```
The only problem here is to introduce a redundant space if option is empty. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74027475
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,16 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = {
+  extraOptions.get("jdbc.create.table.options") match {
--- End diff --

Thanks Sean. Actually, here I have a little bit hesitation. For example, 
"mergeSchema" which may not be so that similar to the other option name 
(prefixed with "spark"). 
```
val mergedDF = spark.read.option("mergeSchema", 
"true").parquet("data/test_table")
```

How about to use some short name as "createTableOptions"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/14559#discussion_r74026903
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -447,7 +447,16 @@ final class DataFrameWriter[T] private[sql](ds: 
Dataset[T]) {
   // Create the table if the table didn't exist.
   if (!tableExists) {
 val schema = JdbcUtils.schemaString(df, url)
-val sql = s"CREATE TABLE $table ($schema)"
+// To allow certain options to append when create a new table, 
which can be
+// table_options or partition_options.
+// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT 
CHARSET=utf8"
+val createtblOptions = {
+  extraOptions.get("jdbc.create.table.options") match {
+case Some(value) => " " + value
+case None => ""
+  }
+}
+val sql = s"CREATE TABLE $table ($schema)" + createtblOptions
--- End diff --

Yes. so right. will fix that, which looks better as a whole part. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14559: [SPARK-16968]Add additional options in jdbc when ...

2016-08-09 Thread GraceH

GitHub user GraceH opened a pull request:

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

[SPARK-16968]Add additional options in jdbc when creating a new table

## What changes were proposed in this pull request?

In the PR, we just allow the user to add additional options when create a 
new table in JDBC writer. 
The options can be table_options or partition_options.
E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT CHARSET=utf8"

## How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration 
tests, manual tests)
will apply test result soon.



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

$ git pull https://github.com/GraceH/spark jdbc_options

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

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


commit b302b1c7ec75ae1e78d132f7ecdb9bb7f33816d4
Author: GraceH <93113...@qq.com>
Date:   2016-08-09T06:47:51Z

Add additional options in jdbc when creating a new table




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-12-18 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-165957323
  
@andreor14 thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-12-15 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-164991082
  
I leave my thoughts under GraceH#2. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-12-15 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r47732666
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -386,17 +386,21 @@ class StandaloneDynamicAllocationSuite
 // the driver refuses to kill executors it does not know about
 syncExecutors(sc)
 val executors = getExecutorIds(sc)
+val executorIdsBefore = executors.head
 assert(executors.size === 2)
 // kill executor 1, and replace it
 assert(sc.killAndReplaceExecutor(executors.head))
 eventually(timeout(10.seconds), interval(10.millis)) {
   val apps = getApplications()
   assert(apps.head.executors.size === 2)
+  val executorIdsAfter = getExecutorIds(sc).head
+  // make sure the old executors head has been killedAndReplaced.
+  assert(executorIdsBefore != executorIdsAfter)
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
 apps = getApplications()
 assert(apps.head.executors.size === 2)
--- End diff --

how about this?

```java
  val executorKilledAndReplaced = executors.head // The previous head which 
is killed and replaced.
  assert(!sc.killExecutor(executorKilledAndReplaced))
  val executorToKill = executors(1) // The valid executor which is still 
working
  assert(sc.killExecutor(executorToKill))
  val executorAppended = executors(2) // The newly added executor 
(replacement)
  assert(sc.killExecutor(executorAppended))


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-12-10 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-163839364
  
 Thanks @zsxwing. The patch seems to pass all tests. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45939112
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -386,17 +386,21 @@ class StandaloneDynamicAllocationSuite
 // the driver refuses to kill executors it does not know about
 syncExecutors(sc)
 val executors = getExecutorIds(sc)
+val executorIdsBefore = executors.head
 assert(executors.size === 2)
 // kill executor 1, and replace it
 assert(sc.killAndReplaceExecutor(executors.head))
 eventually(timeout(10.seconds), interval(10.millis)) {
   val apps = getApplications()
   assert(apps.head.executors.size === 2)
+  val executorIdsAfter = getExecutorIds(sc).head
+  // make sure the old executors head has been killedAndReplaced.
+  assert(executorIdsBefore != executorIdsAfter)
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
 apps = getApplications()
 assert(apps.head.executors.size === 2)
--- End diff --

since we didn't change the `executors` (essentially it is the 
`executorIdsBefore`)  after the previous assignment. it is still {27}.  
`executors(1) == 28`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-159776246
  
I have added the test case 
https://github.com/GraceH/spark/commit/2e4884c30d9edb0a366e9138cbad8772c5645c5d.
 Please let me know your comments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-159774039
  
@andrewor14 Yes. you are so right. Meanwhile it seems the original 
implementation has waited for a while to check if the replacement is there.  
According to you suggestion, I can add the `executor id comparison` here. And 
it is tested locally. What do you think? 

```Java
val executors = getExecutorIds(sc)
assert(executors.size === 2)
// kill executor 1, and replace it
assert(sc.killAndReplaceExecutor(executors.head))
eventually(timeout(10.seconds), interval(10.millis)) {
  val apps = getApplications()
  assert(apps.head.executors.size === 2)
+ // make sure the old executors head has been killedAndReplaced
+ assert(executors.head != getExecutorIds(sc).head)
}



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-159778011
  
Yes. The replacement is finished.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45937858
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -386,17 +386,21 @@ class StandaloneDynamicAllocationSuite
 // the driver refuses to kill executors it does not know about
 syncExecutors(sc)
 val executors = getExecutorIds(sc)
+val executorIdsBefore = executors.head
 assert(executors.size === 2)
 // kill executor 1, and replace it
 assert(sc.killAndReplaceExecutor(executors.head))
 eventually(timeout(10.seconds), interval(10.millis)) {
   val apps = getApplications()
   assert(apps.head.executors.size === 2)
+  val executorIdsAfter = getExecutorIds(sc).head
+  // make sure the old executors head has been killedAndReplaced.
+  assert(executorIdsBefore != executorIdsAfter)
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
 apps = getApplications()
 assert(apps.head.executors.size === 2)
--- End diff --

since the first attempt is to kill {27}, nothing happens. The 
executors.size should be fine with 2. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45939198
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -386,17 +386,21 @@ class StandaloneDynamicAllocationSuite
 // the driver refuses to kill executors it does not know about
 syncExecutors(sc)
 val executors = getExecutorIds(sc)
+val executorIdsBefore = executors.head
 assert(executors.size === 2)
 // kill executor 1, and replace it
 assert(sc.killAndReplaceExecutor(executors.head))
 eventually(timeout(10.seconds), interval(10.millis)) {
   val apps = getApplications()
   assert(apps.head.executors.size === 2)
+  val executorIdsAfter = getExecutorIds(sc).head
+  // make sure the old executors head has been killedAndReplaced.
+  assert(executorIdsBefore != executorIdsAfter)
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
 apps = getApplications()
 assert(apps.head.executors.size === 2)
--- End diff --

That is why killing nothing, and executors.size == 2. Since the previous 27 
was killed. 
The real kill ({28}) is tested in the following case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-25 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/9796#issuecomment-159824727
  
retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-23 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45685308
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
--- End diff --

@andrewor14 you can find there are two test cases.  I guess the second one 
is that you want.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-19 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45418211
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
--- End diff --

if so, we should not kill excutors.head(27). it should be excutor(1). am I 
right?

ï¿½ï¿½ï¿½ï¿½ Grace from mobile phone

 ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ 
ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½Re: [spark] [SPARK-9552] Return "false" while nothing to 
kill in killExecutors (#9796)
ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½andrewor14
ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½apache/spark
ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½"Huang, Jie"


In 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala<https://github.com/apache/spark/pull/9796#discussion_r45414046>:

> @@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
>  }
>
>  var apps = getApplications()
> -// kill executor 1
> -assert(sc.killExecutor(executors.head))
> +// kill executor 1, and actually nothing to kill
> +assert(!sc.killExecutor(executors.head))


no, the idea is more like the following:

  *   you start with executors {27, 28}
  *   you kill and replace 27, so you end up with executors {28, 29}
  *   now you want to kill 28, this should succeed (but currently it 
doesn't in the tests)

ï¿½ï¿½
Reply to this email directly or view it on 
GitHub<https://github.com/apache/spark/pull/9796/files#r45414046>.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-19 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45418928
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
--- End diff --

according to my understanding, the 1st case tries to kill 27. the 2nd one 
is to kill 28. that is why the first one causes nothing to happen. the latter 
case actually kills the executor successfully.

btw, we donot change the 'val executors' after the first assignment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-19 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45412931
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(!sc.killExecutor(executors.head))
--- End diff --

@andrewor14  The executors.head is assigned beforehand. for example, you 
have two executor ID {27,28}. Then, the first one(id 27) is killed with 
replacement. But I guess the newly created executor cannot be with the same ID. 
After that, you try to kill the header executor (id 27), it should return empty 
list (since 27 has been in the pendingToRemove list). Am I right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-18 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45283707
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -450,7 +450,8 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
 
   /**
* Request that the cluster manager kill the specified executors.
-   * @return whether the kill request is acknowledged.
+   * @return whether the kill request is acknowledged. If list to kill is 
empty, it should return
--- End diff --

thanks. will change that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-18 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45283688
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -408,7 +408,8 @@ private[spark] class ExecutorAllocationManager(
   executorsPendingToRemove.add(executorId)
   true
 } else {
-  logWarning(s"Unable to reach the cluster manager to kill executor 
$executorId!")
+  logWarning(s"Unable to reach the cluster manager to kill executor 
$executorId!" +
+s"Or no executor to kill!")
--- End diff --

1. or maybe we need to change that to "or executor is not eligible to 
kill". since there are two situations to get empty list: a) busy b) kill that 
repeatedly

2. There is not action to take in other parts of the code. The problem here 
is if we not change the API (return type). it is really hard to tell it is not 
acknowledged or no executor to kill. If it is not acknowledged, we should print 
warning, but if no executor to kill, it is the normal case with info level. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-18 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45284000
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -395,8 +395,8 @@ class StandaloneDynamicAllocationSuite
 }
 
 var apps = getApplications()
-// kill executor 1
-assert(sc.killExecutor(executors.head))
+// kill executor 1, and actually nothing to kill
+assert(sc.killExecutor(executors.head) === false)
--- End diff --

because this one is killed in replacement part. 
```java
assert(executors.size === 2)
// kill executor 1, and replace it
assert(sc.killAndReplaceExecutor(executors.head)) //executors.head is 
killed here with replace = true.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Return "false" while nothing to k...

2015-11-18 Thread GraceH

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

https://github.com/apache/spark/pull/9796#discussion_r45283718
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -462,7 +463,8 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
* @param force whether to force kill busy executors
-   * @return whether the kill request is acknowledged.
+   * @return whether the kill request is acknowledged. If list to kill is 
empty, it should return
--- End diff --

thanks a lot. i will change that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-17 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-157604487
  
@andrewor14 @vanzin Thanks all. I will follow that by creating a new patch 
under SPARK-9552.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-17 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-157307254
  
@andrewor14  My bad. Since the `val executors = getExecutorIds(sc)` is 
fetched beforehand. We should not kill `executors.head` again and again (it 
should be executor.head, and then executor(1)). Now, i change the sequence of 
that. 
1. set force = false to ignore the first executor
2. set force = true to force kill that first executor. 

Now it should work. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: Return "false" is nothing to kill in killExecu...

2015-11-17 Thread GraceH

GitHub user GraceH opened a pull request:

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

Return "false" is nothing to kill in killExecutors

In discussion (SPARK-9552), we proposed a force kill in `killExecutors`. 
But if there is nothing to kill, it will return back with true 
(acknowledgement). And then, it causes the certain executor(s) (which is not 
eligible to kill) adding to pendingToRemove list for further actions. 

In this patch, we'd like to change the return semantics. If there is 
nothing to kill, we will return "false". and therefore  all those non-eligible 
executors won't be added to the pendingToRemove list. 

@vanzin @andrewor14 As the follow up of PR#7888, please let me know your 
comments.

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

$ git pull https://github.com/GraceH/spark emptyPendingToRemove

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

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


commit c23f887b62a75415bab74036e78d03b92b1a5541
Author: Grace <jie.hu...@intel.com>
Date:   2015-10-28T15:04:37Z

rebase to master branch

commit dc660f63c416c300bd3da48c6a0b9442633313fc
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-10T07:10:09Z

change the task number to count; change the WithLoads to WithTaskCount

commit 27faa6b70e8332f6f70359739cb417be5be7e31e
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-10T07:14:39Z

keep sparkcontext public API un-changed

commit 8774124e07666d337501d0e79bc27bbb35d78b74
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-10T12:50:46Z

fix compile issue

commit 946ed7e0966b10a706372200551345304d2c8089
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-10T13:04:08Z

fix compile issue

commit feefbfef79e396df0187ae947e1b39c80e810082
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-11T07:10:41Z

keep public API

commit fa3c88ea1033b569b853e7a758587bf84323bd6c
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-11T07:12:32Z

keep public API

commit cb78e5605679bad667e5a93ed24a1d280827d121
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-11T07:14:05Z

remove unnecessary comments

commit 5bcfd8148af20c43a993dfaa5a90597070a7c343
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-11T07:15:35Z

clean code

commit 01c236ad3cb435c8b63f8be59c3f5d099b797cf3
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-11T07:16:24Z

clean code

commit 2108dbfdfa962b40ffabf8874a364e8bf1009f93
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-12T13:53:54Z

refine code

commit c0a1d549e84b110e13e06810a9b64da1f5b3230d
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-12T14:00:12Z

remove unnecessary imports

commit 4b1959f6c7d393f34e145aed5eca3ae28e7b7a83
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-12T14:05:24Z

set sc.killExecutor as force = true

commit 0293d8241df79b9c60ebefb6e18c89456be30547
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-12T14:20:44Z

use force = false to do the unittest

commit 342a59d34e76c55f2fc306b45d8510c74cf73af8
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-13T03:32:06Z

refine the unit test & change semantics for force == true only

commit 806a64d8e360e4589f4ad1569cc8c6f379e5987b
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-13T03:43:49Z

refine the unit test & change semantics for force == true only

commit 4ce0ec06d79d2d6a5c68d0415c25c41d19f34736
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-13T03:51:48Z

remove unnecessary imports

commit 551cd2860a85a6895b8c0a3b1815a9a7f108
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-13T04:55:42Z

fix checkstyle issue

commit c44ef8714c6cde70404043e065e81330746f7881
Author: Andrew Or <and...@databricks.com>
Date:   2015-11-13T18:51:48Z

Suggestions

commit d3f51dbfa4d0b9e416c56a69383d19da2699d478
Author: Andrew Or <and...@databricks.com>
Date:   2015-11-13T23:02:51Z

Clean up state in ExecutorAllocationManager

commit 0daeb5a70cea3aa3d0a34189fcaee6fef59d578e
Author: Jie Huang <jie.hu...@intel.com>
Date:   2015-11-17T00:55:20Z

Merge pull request #1 from andrewor14/pr-7888-suggestions

Suggestions for PR 7888

commit 1938e6158838bb1f21884c3d3eba9f0b8ffabe05
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-17T08:29:05Z

fix unittest issue

commit 589083b8d9208cb1044556d0571f4dfd71284263
Author: Grace <jie.hu...@intel.com>
Date:   2015-11-18T07:05:00Z

return false is nothing to kill in killExecutors




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this fe

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r45008522
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala ---
@@ -87,8 +87,8 @@ private[spark] class TaskSchedulerImpl(
   // Incrementing task IDs
   val nextTaskId = new AtomicLong(0)
 
-  // Which executor IDs we have executors on
-  val activeExecutorIds = new HashSet[String]
+  // Number of tasks runing on each executor
--- End diff --

oops. sorry for that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-157224591
  
@andrewor14 That is really a good way to have mock busy status. Thanks a 
lot, really learn a lot from that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r45008783
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -442,7 +452,7 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
   numPendingExecutors += knownExecutors.size
 }
 
-doKillExecutors(executorsToKill)
+(force || !executorsToKill.isEmpty) && doKillExecutors(executorsToKill)
--- End diff --

That is original proposal. I am ok with either of them. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r45008834
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -429,7 +433,13 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
 }
 
 // If an executor is already pending to be removed, do not kill it 
again (SPARK-9795)
-val executorsToKill = knownExecutors.filter { id => 
!executorsPendingToRemove.contains(id) }
+// If this executor is busy, do not kill it unless we are told to 
force kill it (SPARK-9552)
+val executorsToKill = knownExecutors
+  .filter { id => !executorsPendingToRemove.contains(id) }
+  .filter { id => force || !scheduler.isExecutorBusy(id) }
+  // for test only
+  .filter { id => force ||
+
!scheduler.sc.getConf.getBoolean("spark.dynamicAllocation.testing", false)}
--- End diff --

yes. I just wanted to have a mock status.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r45008710
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -404,6 +404,33 @@ class StandaloneDynamicAllocationSuite
 assert(apps.head.getExecutorLimit === 1)
   }
 
+  test("disable force kill for busy executors (SPARK-9552)") {
+sc = new SparkContext(appConf.set("spark.dynamicAllocation.testing", 
"true"))
+val appId = sc.applicationId
+eventually(timeout(10.seconds), interval(10.millis)) {
+  val apps = getApplications()
+  assert(apps.size === 1)
+  assert(apps.head.id === appId)
+  assert(apps.head.executors.size === 2)
+  assert(apps.head.getExecutorLimit === Int.MaxValue)
+}
+// sync executors between the Master and the driver, needed because
+// the driver refuses to kill executors it does not know about
+syncExecutors(sc)
+var executors = getExecutorIds(sc)
+assert(executors.size === 2)
+// force kill busy executor
+assert(killExecutorWithForce(sc, executors.head))
+var apps = getApplications()
+// kill executor successfully
+assert(apps.head.executors.size === 1)
+// try to kill busy executor but it should be failed
+assert(killExecutorWithForce(sc, executors.head, false) === false)
--- End diff --

make sense. :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r45008692
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala
 ---
@@ -455,6 +482,19 @@ class StandaloneDynamicAllocationSuite
 sc.killExecutors(getExecutorIds(sc).take(n))
   }
 
+  private def killExecutorWithForce(
+  sc: SparkContext,
+  executorId: String,
+  force: Boolean = true): Boolean = {
--- End diff --

I did that (i.e., syncExecutors) outside of this function. since this 
function is only used in newly added test, that is why not to choose the name 
of `killExecutor`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-16 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-157227255
  
@vanzin Also thanks for helping me to clarify the thoughts for 
acknowledgement part. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-12 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-156078896
  
@vanzin After changing the semantics in `killExecutors()`, it causes 
certain unit test failure. Since the original expectation is even 
`executorsToKill.isEmpty`, it will return the acknowledge with true. But not, 
it changes the default behavior. We may need to revert to the rescue way. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-12 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44746716
  
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -1489,7 +1489,7 @@ class SparkContext(config: SparkConf) extends Logging 
with ExecutorAllocationCli
   private[spark] def killAndReplaceExecutor(executorId: String): Boolean = 
{
 schedulerBackend match {
   case b: CoarseGrainedSchedulerBackend =>
-b.killExecutors(Seq(executorId), replace = true)
+b.killExecutors(Seq(executorId), replace = true, force = false)
--- End diff --

yes. If it for dead executor, we should force kill that and replace a new 
one. No matter the executor is busy or not. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-12 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-156315327
  
@vanzin and @andrewor14 , please let me know your further imports. sorry 
for certain rounds of amendments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-12 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-156109836
  
@andrewor14, @vanzin 
1.  I have changed `sparkcontext.killExecutors` as `force = true`. 
2. And keep the current public APIs
3. Add a simple unit test to test `force = false`
4. keep the semantics of killExecutor, since empty list to kill action will 
receive `true` acknowledgement.  

Please let me know if you have further comments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-12 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-156296415
  
@vanzin  My bad. I change the code a little bit as below. Only force == 
true will change the semantics, i.e., to return back false when 
`executorsToKill.isEmpty`. 

```java
(force || !executorsToKill.isEmpty) && doKillExecutors(executorsToKill)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-11 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44612985
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala ---
@@ -341,7 +344,10 @@ private[spark] class TaskSchedulerImpl(
   case Some(taskSet) =>
 if (TaskState.isFinished(state)) {
   taskIdToTaskSetManager.remove(tid)
-  taskIdToExecutorId.remove(tid)
+  taskIdToExecutorId.remove(tid) match {
+case Some(execId) => 
activeExecutorIdsWithTaskCount(execId) -= 1
+case None =>
+  }
--- End diff --

I will change that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-11 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44612998
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala ---
@@ -88,7 +88,8 @@ private[spark] class TaskSchedulerImpl(
   val nextTaskId = new AtomicLong(0)
 
   // Which executor IDs we have executors on
-  val activeExecutorIds = new HashSet[String]
+  // each executor will record running or launched task count
+  val activeExecutorIdsWithTaskCount = new HashMap[String, Int]
--- End diff --

Make sense. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-11 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44612960
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -410,8 +410,9 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
* Request that the cluster manager kill the specified executors.
* @return whether the kill request is acknowledged.
*/
-  final override def killExecutors(executorIds: Seq[String]): Boolean = 
synchronized {
-killExecutors(executorIds, replace = false)
+  final override def killExecutors(
+  executorIds: Seq[String]): Boolean = synchronized {
--- End diff --

sorry for missing that. i will revert that back.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-11 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44612916
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -419,17 +420,32 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to force kill busy executors
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(
+  executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
 logInfo(s"Requesting to kill executor(s) ${executorIds.mkString(", 
")}")
 val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
 unknownExecutors.foreach { id =>
   logWarning(s"Executor to kill $id does not exist!")
 }
 
+// force killing all busy and idle executors
+// otherwise, only idle executors are valid to be killed
+val idleExecutors =
+  if (force) {
+knownExecutors
+  } else {
+knownExecutors.filter { id =>
+  logWarning(s"Busy executor $id is not valid to be killed!")
+  !scheduler.isExecutorBusy(id)}
+  }
--- End diff --

Nice suggestion. will change that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-11 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155963074
  
@andrewor14 Here is the problem. Since we didn't provide public API with 
force control. It is impossible to add `force = true` into ` 
b.killExecutors(executorIds)` below. This will change the behavior of 
`sc.killExecutors()`. 

```java
override def killExecutors(executorIds: Seq[String]): Boolean = {
schedulerBackend match {
  case b: CoarseGrainedSchedulerBackend =>
b.killExecutors(executorIds)
  case _ =>
logWarning("Killing executors is only supported in 
coarse-grained mode")
false
}
  }


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44482609
  
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -1489,7 +1493,7 @@ class SparkContext(config: SparkConf) extends Logging 
with ExecutorAllocationCli
   private[spark] def killAndReplaceExecutor(executorId: String): Boolean = 
{
 schedulerBackend match {
   case b: CoarseGrainedSchedulerBackend =>
-b.killExecutors(Seq(executorId), replace = true)
+b.killExecutors(Seq(executorId), replace = true, true)
--- End diff --

make sense, will do.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155602453
  
Thanks @andrewor14. I will cleanup the API stuffs, and meanwhile, to add 
certain unit tests. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44482560
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -442,6 +458,7 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
   numPendingExecutors += knownExecutors.size
 }
 
+// executorsToKill may be empty
--- End diff --

sure. will do


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44493155
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

here is the problem.

1. you have executor-1,-2,-3 to be killed (say timeout triggers that)
2. according to our new criteria, only executor-1 is eligible to kill. 
and -2,-3 are filtered out (force = false), and not to pass to 
`killExecutors`. Only executor-1 send out killing command, and return back its 
acknowledgement.
3.  we get the acknowledgement (actually it only works for executor-1). and 
the current code path will add all executorID(-1,-2,-3) to 
`executorsPendingToRemove`. but actually, only -1 is the real killing case.

In the dynamic allocation, we can do that hypothesis, since it only kills 
single executor each time. But for multiple executor case, there is no chance 
to tell the difference between executorIDs(to kill) and actual idle ones. 
Otherwise, we need to change the APIs to return back what the really killed 
executor-list.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44496393
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

I see your point, if it is ok to change the return value. BTW, for the 
killExecutors(multiple executor ids), shall we add the acknowledged executor to 
`executorsPendingToRemove`? If not, that will be also strange.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155689636
  
@vanzin @andrewor14  I have changed code accordingly. Please let me know 
your comments. Meanwhile, I will try to add unit tests.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44490668
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

@vanzin Here is the code path.

1. Prepare entire executorID-list to be killed (meet certain criteria) 
2. killExecutors will filter out non-eligible ones (some of them may not be 
killed accordingly)
3. no matter what kind of executors filtered out, if some of them are 
acknowledged(really killed), we will add all of the executorID-list to 
`executorsPendingToRemove`. There is no way to tell who is actually to kill. 

That is why we need such kind of rescuing. please let me know if it makes 
sense. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155436527
  
@vanzin  I have changed the patch according to you comments. The only left 
is the return value for `killExecutor`. Please let me understand your thoughts. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155409327
  
@vanzin Sorry. I missed one important thing. The `ExecutorAllocationClient` 
defines `killExecutors()` API for both sparkcontext and 
CoarseGrainedSchedulerBackend. It might be a little bit difficult stick on the 
original sparkcontext API. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44501678
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

OK. I see. You mean change the killExecutor return value only, right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44501893
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

OK. I will change that accordingly. It will change the original semantics 
also. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44497839
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

Yes. I know that. From the API design and implementation (named as 
`killExecutors`),  I'd prefer more general case. In case someone else calls 
that in the future. Besides, batch kill is better than kill them one by one 
each time. If it is ok not to take that account, I will handle that according 
to existing case. 

Thanks for your comments and suggestions. I will change the code 
accordingly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-10 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44499652
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

@vanzin I got little bit confused. If at least one executor was killed, and 
return true. Then all those executors will be added to 
`executorsPendingToRemove`. see 
https://github.com/apache/spark/blob/f85aa06464a10f5d1563302fd76465dded475a12/core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala#L405
 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-09 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155333294
  
@vanzin  Got your point. I will follow that by eliminating the secondary 
option in public API. thanks for the confirmation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-09 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r44375390
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -509,6 +511,13 @@ private[spark] class ExecutorAllocationManager(
   private def onExecutorBusy(executorId: String): Unit = synchronized {
 logDebug(s"Clearing idle timer for $executorId because it is now 
running a task")
 removeTimes.remove(executorId)
+
+// Executor is added to remove by misjudgment due to async listener 
making it as idle).
+// see SPARK-9552
+if (executorsPendingToRemove.contains(executorId)) {
--- End diff --

@vanzin, In the original design, I changed the return back value for that 
function (`killExecutors`).  Not only for it is the last round of review 
comments. But also since It is still a little bit strange. For example, you 
have 3 executors to kill with force=false. And you find one of them is busy. It 
is hard to tell killing success or not directly. But if we only support single 
executor here, it is much simple and straightforward. 
Besides, this is changed according to last round of review comments. Since 
the killExecutors only returns with the acknowledge (in documentation), which 
doesn't indicate the status of kill action.  Please let me know your further 
thoughts.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-09 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-155274058
  
@vanzin Regarding that public API, if it is not necessary to enable the 
force control, I will move that option. Basically, it is an additional option 
with default value. It is quite free for end user to call that in original way 
or with 2 parameters.  Please let me know your feedback. thanks. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-08 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-154929972
  
Thanks @vanzin for the comments. I will change the stuffs accordingly. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-11-05 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-154322010
  
Thanks @andrewor14. 

Hi @vanzin,  Let me give a quick brief to you about the patch and its goal. 

There is a bug in dynamic allocation. Since some of the busy executors 
might be killed by "mistake", when we met such kind of situation in real-world 
deployment frequently. 
1. The executor is being idled for 60 seconds, and just marked as to be 
killed by dynamic allocation criteria.
2. The scheduler is assigning one/several tasks to that executor. The 
listener event not reached that time. (since the listener event only happens 
after new tasks assigned synchronously)
3.  The executor is killed as planned. But actually, that executor is just 
assigned with some tasks. That causes one busy executor is killed by 
âmistake".

To solve this problem, one thing is to make that task assignment and 
notification synchronized. But this approach is not suitable for current 
implementation (listener mechanism). 

Here I proposed another way. To add the force control in killExecutor(). 
For dynamic allocation, we need to check if the executor is busy or not before 
really taking the kill action. By doing so, even the listen event not arrives 
in time, we can actively rescue certain busy executors (to be killed but with 
new tasks assigned).  Thru dynamic allocation we should not kill those busy 
executors (disable force control).

And meanwhile, we open that force control to the end user (sparkcontext 
public API). The end user can decide whether to force kill certain executors .

Please let me understand your thoughts. Thanks a lot.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-10-28 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-151875147
  
@andrewor14 I have tried to rebase the original proposal to latest master 
branch. Please let me know if you have further question or concern. Thanks a 
lot.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-08 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-138573813
  
@andrewor14  I have pushed another proposal.  Please let me know your 
comments.
 * The SparkContext allows end-user to set `force` control while 
killExecutor(s). Dynamic allocation will always uses force control as false to 
avoid false killing while executor is busy.
 * The `killExectutors` log out some executor busy, and cannot be killed if 
`force==false
 * The `killExectutors` return back acknowledge no matter it has executor 
to kill
 * The onTaskStart (i.e., `OnExecutorBusy`) will rescuer certain executor 
from `pendingToRemove` list if it is busy and added to that list by misjudgment.
 * Add one HashMap for all those `activeExecutors`, which records the 
running/launched task number. If the task number > 0, that executor is busy. 
And `isExecutorBusy` returns back true.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-07 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r38890521
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -413,25 +413,38 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to kill busy executors (who are running tasks)
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(
+  executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
 logInfo(s"Requesting to kill executor(s) ${executorIds.mkString(", 
")}")
 val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
 unknownExecutors.foreach { id =>
   logWarning(s"Executor to kill $id does not exist!")
 }
 
+val idleExecutors = {
+  if (force) {
+knownExecutors
+  } else {
+knownExecutors.filter(executor =>
+  !scheduler.taskIdToExecutorId.exists(_._2 == executor))
+  }
+}
 // If we do not wish to replace the executors we kill, sync the target 
number of executors
 // with the cluster manager to avoid allocating new ones. When 
computing the new target,
 // take into account executors that are pending to be added or removed.
 if (!replace) {
   doRequestTotalExecutors(numExistingExecutors + numPendingExecutors
-- executorsPendingToRemove.size - knownExecutors.size)
+- executorsPendingToRemove.size - idleExecutors.size)
 }
 
-executorsPendingToRemove ++= knownExecutors
-doKillExecutors(knownExecutors)
+executorsPendingToRemove ++= idleExecutors
+// return false: there has some busyExecutors or killing certain 
executor failed
+doKillExecutors(idleExecutors) && idleExecutors.size == 
knownExecutors.size
--- End diff --

@andrewor14  The problems here are, 

 * if there is not idleExecutor to be killed, shall we return back with 
acknowledged?
 * It is quite tricky to have the force control for `killExecutors`. For 
example, we have 3 executors to kill. But  only one of them are idle. Shall we 
return true to the end user?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-06 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r38828688
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -413,25 +413,38 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to kill busy executors (who are running tasks)
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(
+  executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
 logInfo(s"Requesting to kill executor(s) ${executorIds.mkString(", 
")}")
 val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
 unknownExecutors.foreach { id =>
   logWarning(s"Executor to kill $id does not exist!")
 }
 
+val idleExecutors = {
+  if (force) {
+knownExecutors
+  } else {
+knownExecutors.filter(executor =>
+  !scheduler.taskIdToExecutorId.exists(_._2 == executor))
--- End diff --

I knew this step was costly. I will add one more tracking set if it allows.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-06 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r38828767
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -413,25 +413,38 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to kill busy executors (who are running tasks)
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(
+  executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
 logInfo(s"Requesting to kill executor(s) ${executorIds.mkString(", 
")}")
 val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
 unknownExecutors.foreach { id =>
   logWarning(s"Executor to kill $id does not exist!")
 }
 
+val idleExecutors = {
+  if (force) {
+knownExecutors
+  } else {
+knownExecutors.filter(executor =>
+  !scheduler.taskIdToExecutorId.exists(_._2 == executor))
+  }
+}
 // If we do not wish to replace the executors we kill, sync the target 
number of executors
 // with the cluster manager to avoid allocating new ones. When 
computing the new target,
 // take into account executors that are pending to be added or removed.
 if (!replace) {
   doRequestTotalExecutors(numExistingExecutors + numPendingExecutors
-- executorsPendingToRemove.size - knownExecutors.size)
+- executorsPendingToRemove.size - idleExecutors.size)
 }
 
-executorsPendingToRemove ++= knownExecutors
-doKillExecutors(knownExecutors)
+executorsPendingToRemove ++= idleExecutors
+// return false: there has some busyExecutors or killing certain 
executor failed
+doKillExecutors(idleExecutors) && idleExecutors.size == 
knownExecutors.size
--- End diff --

OK. will do.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-06 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-138149073
  
@andrewor14  Thanks for the feedback. I will take a look at your comments, 
and to revise the code accordingly. any concern, will let you know. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-09-06 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-138161276
  
@andrewor14 Thanks for the comments. 

Regarding #1, very good point. That's why I try to return back false if 
force-killing failed. This is the simplest way. That `executorID` won't be 
added to `executorsPendingToRemove.add(executorId)`. See 
https://github.com/GraceH/spark/blob/forcekill/core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala#L410.
 The only concern here is that it somehow changes the semantics for that return 
value.  

Regarding #2, Nice suggestion. That's also my thoughts too. The end user 
can force kill any executor by setting force=true. I will make private function 
for `ExecutorAllocationManager`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9879][SQL][WIP] Fix OOM in Limit clause...

2015-08-13 Thread GraceH

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

https://github.com/apache/spark/pull/8128#discussion_r37046803
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala ---
@@ -224,6 +225,56 @@ case class Limit(limit: Int, child: SparkPlan)
 
 /**
  * :: DeveloperApi ::
+ * Take the first limit elements. and the limit can be any number less 
than Integer.MAX_VALUE.
+ * If it is terminal and is invoked using executeCollect, it probably 
cause OOM if the
+ * records number is large enough. Not like the Limit clause, this 
operator will not change
+ * any partitions of its child operator.
+ */
+@DeveloperApi
+case class LargeLimit(limit: Int, child: SparkPlan)
+  extends UnaryNode {
+  /** We must copy rows when sort based shuffle is on */
+  private def sortBasedShuffleOn = 
SparkEnv.get.shuffleManager.isInstanceOf[SortShuffleManager]
+
+  override def output: Seq[Attribute] = child.output
+
+  override def executeCollect(): Array[Row] = child.executeTake(limit)
+
+  protected override def doExecute(): RDD[InternalRow] = {
+val rdd = if (sortBasedShuffleOn) {
+  child.execute().map(_.copy()).persist(StorageLevel.MEMORY_AND_DISK)
+} else {
+  child.execute().persist(StorageLevel.MEMORY_AND_DISK)
+}
+
+// We assume the maximize record number in a partition is less than 
Integer.MAX_VALUE
+val partitionRecordCounts = rdd.mapPartitions({ iterator =
+  Iterator(iterator.count(_ = true))
+}, true).collect()
+
+var totalSize = 0
+// how many records we have to take from each partition
+val requiredRecordCounts = partitionRecordCounts.map { count =
--- End diff --

just minor suggestion. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9879][SQL][WIP] Fix OOM in Limit clause...

2015-08-13 Thread GraceH

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

https://github.com/apache/spark/pull/8128#discussion_r37044861
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala ---
@@ -224,6 +225,56 @@ case class Limit(limit: Int, child: SparkPlan)
 
 /**
  * :: DeveloperApi ::
+ * Take the first limit elements. and the limit can be any number less 
than Integer.MAX_VALUE.
+ * If it is terminal and is invoked using executeCollect, it probably 
cause OOM if the
+ * records number is large enough. Not like the Limit clause, this 
operator will not change
+ * any partitions of its child operator.
+ */
+@DeveloperApi
+case class LargeLimit(limit: Int, child: SparkPlan)
+  extends UnaryNode {
+  /** We must copy rows when sort based shuffle is on */
+  private def sortBasedShuffleOn = 
SparkEnv.get.shuffleManager.isInstanceOf[SortShuffleManager]
+
+  override def output: Seq[Attribute] = child.output
+
+  override def executeCollect(): Array[Row] = child.executeTake(limit)
+
+  protected override def doExecute(): RDD[InternalRow] = {
+val rdd = if (sortBasedShuffleOn) {
+  child.execute().map(_.copy()).persist(StorageLevel.MEMORY_AND_DISK)
+} else {
+  child.execute().persist(StorageLevel.MEMORY_AND_DISK)
+}
+
+// We assume the maximize record number in a partition is less than 
Integer.MAX_VALUE
+val partitionRecordCounts = rdd.mapPartitions({ iterator =
+  Iterator(iterator.count(_ = true))
+}, true).collect()
+
+var totalSize = 0
+// how many records we have to take from each partition
+val requiredRecordCounts = partitionRecordCounts.map { count =
--- End diff --

Will it be more efficient to use loop?  For example: you have 1000 
partition count (100, 4, 5, 700, 10 ...), and limit number is 10.  If to use 
loop, you will do the calculation once. But if to choose map, it will do 1000 
times. 
Besides, maybe we can save the storage space for requiredRecordCounts.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9879][SQL][WIP] Fix OOM in Limit clause...

2015-08-13 Thread GraceH

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

https://github.com/apache/spark/pull/8128#discussion_r37045748
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala ---
@@ -224,6 +225,56 @@ case class Limit(limit: Int, child: SparkPlan)
 
 /**
  * :: DeveloperApi ::
+ * Take the first limit elements. and the limit can be any number less 
than Integer.MAX_VALUE.
+ * If it is terminal and is invoked using executeCollect, it probably 
cause OOM if the
+ * records number is large enough. Not like the Limit clause, this 
operator will not change
+ * any partitions of its child operator.
+ */
+@DeveloperApi
+case class LargeLimit(limit: Int, child: SparkPlan)
+  extends UnaryNode {
+  /** We must copy rows when sort based shuffle is on */
+  private def sortBasedShuffleOn = 
SparkEnv.get.shuffleManager.isInstanceOf[SortShuffleManager]
+
+  override def output: Seq[Attribute] = child.output
+
+  override def executeCollect(): Array[Row] = child.executeTake(limit)
+
+  protected override def doExecute(): RDD[InternalRow] = {
+val rdd = if (sortBasedShuffleOn) {
+  child.execute().map(_.copy()).persist(StorageLevel.MEMORY_AND_DISK)
+} else {
+  child.execute().persist(StorageLevel.MEMORY_AND_DISK)
--- End diff --

Beside it is very hard to tell what kind of storage level to pick up. 
Another option may be to add document as the first step and mark this as TODO 
item.   If your limit size is larger than LIMIT, you should run with 
LargeLimit. However, it may bring certain performance loss. But at least, you 
can finish the query without any OOM exception. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9591][CORE]Job may fail for exception d...

2015-08-09 Thread GraceH

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

https://github.com/apache/spark/pull/7927#discussion_r36596179
  
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala 
---
@@ -590,10 +590,21 @@ private[spark] class BlockManager(
   private def doGetRemote(blockId: BlockId, asBlockResult: Boolean): 
Option[Any] = {
 require(blockId != null, BlockId is null)
 val locations = Random.shuffle(master.getLocations(blockId))
+var failTimes = 0
 for (loc - locations) {
   logDebug(sGetting remote block $blockId from $loc)
-  val data = blockTransferService.fetchBlockSync(
-loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  val data = try {
+blockTransferService.fetchBlockSync(
+  loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  } catch {
+case e: IOException if failTimes  locations.size - 1 =
+  // Return null when IOException throw, so we can fetch block
+  // from another location if there still have locations
+  failTimes += 1
+  logWarning(sTry ${failTimes} times getting remote block 
$blockId from $loc failed:, e)
--- End diff --

That's fine @squito. Both option (a) and (b) are acceptable. 

BTW, we'd better to add some document to tell the caller, it throws out an 
Exception. In Java, all expected exceptions are displayed explicitly. It is 
easy to understand which kind of exceptions to be catch in the caller. It seems 
most of the caller for ```bm.getRemoteBytes``` or ```bm.get``` replying on the 
returned back ```Option```, and not being aware of any exception. We can tell 
them if getting blocks failed from all remotes, it  is expected one new 
```Exception``` there as Unit test does. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9591][CORE]Job may fail for exception d...

2015-08-05 Thread GraceH

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

https://github.com/apache/spark/pull/7927#discussion_r36372781
  
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala 
---
@@ -590,10 +590,21 @@ private[spark] class BlockManager(
   private def doGetRemote(blockId: BlockId, asBlockResult: Boolean): 
Option[Any] = {
 require(blockId != null, BlockId is null)
 val locations = Random.shuffle(master.getLocations(blockId))
+var failTimes = 0
 for (loc - locations) {
   logDebug(sGetting remote block $blockId from $loc)
-  val data = blockTransferService.fetchBlockSync(
-loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  val data = try {
+blockTransferService.fetchBlockSync(
+  loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  } catch {
+case e: IOException if failTimes  locations.size - 1 =
+  // Return null when IOException throw, so we can fetch block
+  // from another location if there still have locations
+  failTimes += 1
+  logWarning(sTry ${failTimes} times getting remote block 
$blockId from $loc failed:, e)
--- End diff --

@squito We'd better to catch up the exception to avoid working flow to be 
broken.   The single fetch attempt should not break the entire code path. No 
matter what the exception type is.

Regarding the place to catch that exception. For the debugging or 
diagnosis, we'd better to mark down how many remote fetch failures there and 
why. And log out all the fetches are failed or not.  It seems to be more 
reasonable to catch the exception in ```getRemote```. ```getRemote``` itself 
can tolerant  certain fetch failures from parts of the remotes. It only 
requires single success. That is the design for ```getRemote```.

The ```fetchBlockSync``` is a function to tell if fetch fail or success. If 
success, return back the data. If not, throw out the exception to indicate why. 
To swallow the exception there seems not so that reasonable. The upper level 
function can decide if the exception or fetch failure is acceptable or not. 

What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-04 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-127855211
  
It seems the test failure not related to this PR 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9591][CORE]Job may fail for exception d...

2015-08-04 Thread GraceH

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

https://github.com/apache/spark/pull/7927#discussion_r36271550
  
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala 
---
@@ -592,8 +592,14 @@ private[spark] class BlockManager(
 val locations = Random.shuffle(master.getLocations(blockId))
 for (loc - locations) {
   logDebug(sGetting remote block $blockId from $loc)
-  val data = blockTransferService.fetchBlockSync(
-loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  val data = try {
+blockTransferService.fetchBlockSync(
+  loc.host, loc.port, loc.executorId, 
blockId.toString).nioByteBuffer()
+  } catch {
+case e: Throwable =
+  logWarning(sException during getting remote block $blockId from 
$loc, e)
--- End diff --

@squito So agree to do like ```askWithRetry```.  If we can get one block 
from any remote store successfully, it successes. We should not break the 
working path whenever meet the first exception.

So maybe, we need to catch all kinds of Exceptions (not IOException only). 
If some attempts failed, we need to log out the exception information but 
continue the fetching work.  When we run to the final location and it still 
throws out certain exception, we need to throw out a NEW exception to tell that 
all attempts failed (i.e., no available location there). and meanwhile, maybe 
to add the last exception information into this NEW exception. 

But if we only focus IOException, when we meet some types of exceptions for 
certain locations, it still breaks the entire workflow (to fetch data from the 
rest locations if possible). 

What do you think?  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-03 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-127445692
  
@CodingCat  Sorry for the ambiguous words in the description. In general, 
the patch aims to fix the false killing bug in dynamic allocation. And at the 
same time, we leave a chance to have more options in ```killExecutors```.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-03 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r36149140
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -413,25 +413,38 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to kill busy executors (who are running tasks)
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
--- End diff --

Thanks. I will fix that. :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-03 Thread GraceH

Github user GraceH commented on the pull request:

https://github.com/apache/spark/pull/7888#issuecomment-127444561
  
@CodingCat  What I mean is to add the force control in the 
```killExecutors``` API.  Currently, the dynamic allocation is using that API 
with force=false (I suppose we should not kill working executors in Dynamic 
allocation). And for others, they are free to use that option as true or false. 
 If they really want to do that, they can call the private API by setting that 
as ```true```.

Regarding the public API for the users, we'd better have a discussion if to 
add a new public API (it is a little bit out of this PR's scope). From my 
perspective, to modify the exiting public API is not a good idea. It may cause 
compatibility issue. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-03 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r36148284
  
--- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
@@ -264,10 +264,10 @@ private[spark] class ExecutorAllocationManager(
 updateAndSyncNumExecutorsTarget(now)
 
 removeTimes.retain { case (executorId, expireTime) =
-  val expired = now = expireTime
+  var expired = now = expireTime
   if (expired) {
-initializing = false
-removeExecutor(executorId)
+expired = removeExecutor(executorId)
+if (expired)  initializing = false
--- End diff --

I have done the style check before committing. sorry for missing the 
```if``` block here.  I will fix that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-9552] Add force control for killExecuto...

2015-08-03 Thread GraceH

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

https://github.com/apache/spark/pull/7888#discussion_r36148319
  
--- Diff: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ---
@@ -413,25 +413,38 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
*
* @param executorIds identifiers of executors to kill
* @param replace whether to replace the killed executors with new ones
+   * @param force whether to kill busy executors (who are running tasks)
* @return whether the kill request is acknowledged.
*/
-  final def killExecutors(executorIds: Seq[String], replace: Boolean): 
Boolean = synchronized {
+  final def killExecutors(executorIds: Seq[String],
+  replace: Boolean,
+  force: Boolean): Boolean = synchronized {
 logInfo(sRequesting to kill executor(s) ${executorIds.mkString(, 
)})
 val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
 unknownExecutors.foreach { id =
   logWarning(sExecutor to kill $id does not exist!)
 }
 
+val idleExecutors = {
+  if (force) {
+knownExecutors
+  } else {
+knownExecutors.filter{ executor =
--- End diff --

I will replace ```{}``` by ```()```. thanks all.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: Add force control for killExecutors to avoid f...

2015-08-03 Thread GraceH

GitHub user GraceH opened a pull request:

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

Add force control for killExecutors to avoid false killing for those busy 
executors

By using the dynamic allocation, sometimes it occurs false killing for 
those busy executors. Some executors with assignments will be killed because of 
being idle for enough time (say 60 seconds). The root cause is that the 
Task-Launch listener event is asynchronized.  

For example, some executors are under assigning tasks, but not sending out 
the listener notification yet. Meanwhile, the dynamic allocation's executor 
idle time is up (e.g., 60 seconds). It will trigger killExecutor event at the 
same time. 
 1. the timer expiration starts before the listener event arrives.
 2. Then, the task is going to run on top of that killed/killing executor. 
It will lead to task failure finally. 

Here is the proposal to fix it. We can add the force control for 
killExecutor. If the force control is not set (i.e., false), we'd better to 
check if the executor under killing is idle or busy. If the current executor 
has some assignment, we should not kill that executor and return back false (to 
indicate killing failure). In dynamic allocation, we'd better to turn off force 
killing (i.e., force = false), we will meet killing failure if tries to kill a 
busy executor. And then, the executor timer won't be invalid. Later on, the 
task assignment event arrives, we can remove the idle timer accordingly. So 
that we can avoid false killing for those busy executors in dynamic allocation. 

For the rest of usages, the end users can decide if to use force killing or 
not by themselves.  If to turn on that option, the killExecutor will do the 
action without any status checking.

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

$ git pull https://github.com/GraceH/spark forcekill

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

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


commit 4acbd79a2934126c045ce6c4a8f9133dac4c062a
Author: Grace jie.hu...@intel.com
Date:   2015-08-03T06:20:09Z

Add force control for killExecutors




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

1 2 >

1 - 100 of 113 matches

Mail list logo