[SPARK-14400] [SQL] ScriptTransformation does not fail the job for bad user command #12194

tejasapatil · 2016-04-06T02:07:21Z

What changes were proposed in this pull request?

Refer to the Jira for the problem: jira : https://issues.apache.org/jira/browse/SPARK-14400
The fix is to check if the process has exited with a non-zero exit code in hasNext(). I have moved this and checking of writer thread exception to a separate method.

How was this patch tested?

Ran a job which had incorrect transform script command and saw that the job fails
Existing unit tests for ScriptTransformationSuite. Added a new unit test

tejasapatil · 2016-04-06T02:08:58Z

ok to test

SparkQA · 2016-04-06T03:15:54Z

Test build #55074 has finished for PR 12194 at commit df48e1e.

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

rxin · 2016-04-06T03:23:06Z

cc @srowen

SparkQA · 2016-04-08T16:33:54Z

Test build #55358 has finished for PR 12194 at commit e3899c3.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

tejasapatil · 2016-04-08T16:46:04Z

ok to test

SparkQA · 2016-04-08T18:00:25Z

Test build #55360 has finished for PR 12194 at commit ebb5ea1.

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

tejasapatil · 2016-04-13T01:45:00Z

ok to test

SparkQA · 2016-04-13T03:07:12Z

Test build #55676 has finished for PR 12194 at commit 1054b71.

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

tejasapatil · 2016-05-10T22:25:37Z

ok to test

SparkQA · 2016-05-10T23:15:57Z

Test build #58286 has finished for PR 12194 at commit e37e0aa.

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

tejasapatil · 2016-05-17T21:04:51Z

ok to test

SparkQA · 2016-05-17T21:49:52Z

Test build #58720 has finished for PR 12194 at commit 524eb71.

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

tejasapatil · 2016-05-18T06:59:24Z

ok to test

SparkQA · 2016-05-18T08:42:36Z

Test build #58750 has finished for PR 12194 at commit abd65d8.

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

tejasapatil · 2016-05-18T13:48:04Z

Can anyone please review this PR ?

tejasapatil · 2016-05-22T18:40:36Z

Can anyone please review this PR ?

sameeragarwal · 2016-05-24T23:52:48Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformation.scala

unrelated to this PR but we should probably only print the partial contents of the circular buffer if the number of bytes written to it are less than its total size

@sameeragarwal : Thanks for pointing that out. I will submit a separate PR for that.

here : #13351

sameeragarwal · 2016-05-24T23:56:43Z

This patch looks good to me. @srowen / @JoshRosen can one of you please take a second look as well?

rxin · 2016-05-25T01:21:06Z

@srowen want to review this? Given you reviewed the last transform pr.

rxin · 2016-05-25T01:22:23Z

@tejasapatil on a related note, we'd want to remove as much hive code dependency as possible. One command that is left is ScriptTransformation. Would you have time to implement this in sql/core without Hive dependency for ScriptTransformation? It seems like one of the primary operators you guys would use.

srowen · 2016-05-26T01:39:50Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformation.scala

Hm, is it me or does this get hard to follow the return values. Generally the method returns "true" unless one of several conditions caused it to decide it was finished earlier. Those could be handled with early "return false" rather than lots of "else ... true" branches.

yea i think that's a great idea. the level of nesting is a little bit too much here

+1. Did the change

srowen · 2016-05-26T01:41:12Z

With one minor suggestion this LGTM

rxin · 2016-05-26T22:37:00Z

@tejasapatil want to update this so we can merge it for 2.0?

- Added test case

…read might be the first to see the effect of process being killed ``` Exception in thread "Thread-ScriptTransformation-Feed" java.io.IOException: Broken pipe at java.io.FileOutputStream.writeBytes(Native Method) at java.io.FileOutputStream.write(FileOutputStream.java:326) at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:82) at java.io.BufferedOutputStream.flush(BufferedOutputStream.java:140) at java.io.FilterOutputStream.close(FilterOutputStream.java:158) at org.apache.spark.sql.hive.execution.ScriptTransformationWriterThread$$anonfun$run$1.apply$mcV$sp(ScriptTransformation.scala:307) at org.apache.spark.sql.hive.execution.ScriptTransformationWriterThread$$anonfun$run$1.apply(ScriptTransformation.scala:268) at org.apache.spark.sql.hive.execution.ScriptTransformationWriterThread$$anonfun$run$1.apply(ScriptTransformation.scala:268) at org.apache.spark.util.Utils$.logUncaughtExceptions(Utils.scala:1793) at org.apache.spark.sql.hive.execution.ScriptTransformationWriterThread.run(ScriptTransformation.scala:268) ```

tejasapatil · 2016-05-27T06:13:31Z

@rxin : Sorry for delay... was caught up in some other things. I have updated the PR now with the review comments.

Re your suggestion about removing hive code dependency: I will work on it

sameeragarwal · 2016-05-27T07:37:20Z

LGTM pending jenkins. Thanks!

SparkQA · 2016-05-27T07:38:30Z

Test build #59466 has finished for PR 12194 at commit 4a7b2e8.

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

rxin · 2016-05-27T19:05:06Z

Merging in master/2.0.

… user command ## What changes were proposed in this pull request? - Refer to the Jira for the problem: jira : https://issues.apache.org/jira/browse/SPARK-14400 - The fix is to check if the process has exited with a non-zero exit code in `hasNext()`. I have moved this and checking of writer thread exception to a separate method. ## How was this patch tested? - Ran a job which had incorrect transform script command and saw that the job fails - Existing unit tests for `ScriptTransformationSuite`. Added a new unit test Author: Tejas Patil <[email protected]> Closes #12194 from tejasapatil/script_transform. (cherry picked from commit a96e415) Signed-off-by: Reynold Xin <[email protected]>

…tents written if buffer isn't full ## What changes were proposed in this pull request? 1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values 2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over #12194 (comment)) 3. Simplified `toString()` ## How was this patch tested? Added new test case Author: Tejas Patil <[email protected]> Closes #13351 from tejasapatil/circular_buffer. (cherry picked from commit ac38bdc) Signed-off-by: Sean Owen <[email protected]>

…tents written if buffer isn't full ## What changes were proposed in this pull request? 1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values 2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over #12194 (comment)) 3. Simplified `toString()` ## How was this patch tested? Added new test case Author: Tejas Patil <[email protected]> Closes #13351 from tejasapatil/circular_buffer.

…tents written if buffer isn't full 1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values 2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over #12194 (comment)) 3. Simplified `toString()` Added new test case Author: Tejas Patil <[email protected]> Closes #13351 from tejasapatil/circular_buffer. (cherry picked from commit ac38bdc) Signed-off-by: Sean Owen <[email protected]>

…tents written if buffer isn't full 1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values 2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over apache#12194 (comment)) 3. Simplified `toString()` Added new test case Author: Tejas Patil <[email protected]> Closes apache#13351 from tejasapatil/circular_buffer. (cherry picked from commit ac38bdc) Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 714f4d7)

tejasapatil force-pushed the script_transform branch from e3899c3 to ebb5ea1 Compare April 8, 2016 16:43

tejasapatil force-pushed the script_transform branch from ebb5ea1 to 1054b71 Compare April 13, 2016 01:43

tejasapatil force-pushed the script_transform branch from 1054b71 to e37e0aa Compare May 10, 2016 22:25

tejasapatil force-pushed the script_transform branch from e37e0aa to 524eb71 Compare May 17, 2016 21:04

tejasapatil force-pushed the script_transform branch from 524eb71 to abd65d8 Compare May 18, 2016 06:59

sameeragarwal reviewed May 24, 2016
View reviewed changes

srowen reviewed May 26, 2016
View reviewed changes

tejasapatil added 2 commits May 26, 2016 22:28

- Moved the failure check to a method

bacf535

- Added test case

Removed unwanted changes

c672a4c

tejasapatil added 5 commits May 26, 2016 22:28

Made the test case more robust

aa45d1d

rebased

2637039

Handle exceptions in the ScriptTransformationWriterThread

660deb3

simplified the if else nestedness

4a7b2e8

tejasapatil force-pushed the script_transform branch from abd65d8 to 4a7b2e8 Compare May 27, 2016 06:10

tejasapatil mentioned this pull request May 27, 2016

[SPARK-15601][CORE] CircularBuffer's toString() to print only the contents written if buffer isn't full #13351

Closed

asfgit closed this in a96e415 May 27, 2016

[SPARK-14400] [SQL] ScriptTransformation does not fail the job for bad user command #12194

[SPARK-14400] [SQL] ScriptTransformation does not fail the job for bad user command #12194

Uh oh!

Conversation

tejasapatil commented Apr 6, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tejasapatil commented Apr 6, 2016

Uh oh!

SparkQA commented Apr 6, 2016

Uh oh!

rxin commented Apr 6, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

tejasapatil commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

tejasapatil commented Apr 13, 2016

Uh oh!

SparkQA commented Apr 13, 2016

Uh oh!

tejasapatil commented May 10, 2016

Uh oh!

SparkQA commented May 10, 2016

Uh oh!

tejasapatil commented May 17, 2016

Uh oh!

SparkQA commented May 17, 2016

Uh oh!

tejasapatil commented May 18, 2016

Uh oh!

SparkQA commented May 18, 2016

Uh oh!

tejasapatil commented May 18, 2016

Uh oh!

tejasapatil commented May 22, 2016

Uh oh!

sameeragarwal May 24, 2016

Choose a reason for hiding this comment

Uh oh!

tejasapatil May 27, 2016

Choose a reason for hiding this comment

Uh oh!

tejasapatil May 27, 2016

Choose a reason for hiding this comment

Uh oh!

sameeragarwal commented May 24, 2016

Uh oh!

rxin commented May 25, 2016

Uh oh!

rxin commented May 25, 2016

Uh oh!

srowen May 26, 2016

Choose a reason for hiding this comment

Uh oh!

rxin May 26, 2016

Choose a reason for hiding this comment

Uh oh!

tejasapatil May 27, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented May 26, 2016

Uh oh!

rxin commented May 26, 2016

Uh oh!

tejasapatil commented May 27, 2016

Uh oh!

sameeragarwal commented May 27, 2016

Uh oh!

SparkQA commented May 27, 2016

Uh oh!

rxin commented May 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects