Uh oh!
There was an error while loading. Please reload this page.
[SPARK-28260][SQL] Add CLOSED state to ExecutionState - #25062
Conversation
SparkQA
commented
Jul 6, 2019
Test build #107295 has finished for PR 25062 at commit
|
wangyum
commented
Jul 6, 2019
| import org.apache.spark.sql.{DataFrame, Row => SparkRow, SQLContext} | ||
| import org.apache.spark.sql.execution.HiveResult | ||
| import org.apache.spark.sql.execution.command.SetCommand | ||
| import org.apache.spark.sql.hive.thriftserver.HiveThriftServer2.listener |
There was a problem hiding this comment.
style nit: I would avoid importing the listener field directly, as it creates extra changes, and I think it's not common practice in Spark to import field from within an object (except for dsl, like, org.apache.spark.functions).
| } | ||
| def onOperationClosed(id: String): Unit = synchronized { | ||
| executionList(id).finishTimestamp = System.currentTimeMillis |
There was a problem hiding this comment.
I would add a separate field closedTimestamp, and add a separate column in the tables in the UI (overall and within session). Both the time it finished execution, and was closed are interesting information to show - a long time between finished and closed shows that it spent a lot of time returning the result.
What do you think?
There was a problem hiding this comment.
How about?
Execution Time = Finish Time - Start TimeDuration = Close Time - Start Time
There was a problem hiding this comment.
or just add Fetch result time (Fetch result time = Close Time - Finish Time):
| Start Time | Finish Time | Duration | Fetch result time | Statement | State |
|---|
There was a problem hiding this comment.
I would vote for the former - add Close time and Execution time and Duration.Fetch result time is a bit of a long label, and also I think it might be slightly misleading - this may also be the client being slow in processing the results, just sitting on an idle cursor, or actually closing it without fetching all results etc.
Having Duration for the total time gives all the same information and I think it's also more relevant, to have the time from start to close.
SparkQA
commented
Jul 8, 2019
Test build #107347 has finished for PR 25062 at commit
|
juliuszsompolski
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
juliuszsompolski
commented
Jul 8, 2019
cc @gatorsmile , @hvanhovell |
gatorsmile
left a comment
There was a problem hiding this comment.
LGTM
Thanks! Merged to master.
gatorsmile
commented
Jul 12, 2019
It might be misleading to end users. Could you create a doc for Web UI? See the JIRA https://issues.apache.org/jira/browse/SPARK-28373 |
## What changes were proposed in this pull request? The `ThriftServerTab` displays a FINISHED state when the operation finishes execution, but quite often it still takes a lot of time to fetch the results. OperationState has state CLOSED for when after the iterator is closed. This PR add CLOSED state to ExecutionState, and override the `close()` in SparkExecuteStatementOperation, SparkGetColumnsOperation, SparkGetSchemasOperation and SparkGetTablesOperation. ## How was this patch tested? manual tests 1. Add `Thread.sleep(10000)` before [SparkExecuteStatementOperation.scala#L112](https://github.com/apache/spark/blob/b2e7677f4d3d8f47f5f148680af39d38f2b558f0/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala#L112) 2. Switch to `ThriftServerTab`:  3. After a while:  Closesapache#25062 from wangyum/SPARK-28260. Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>
What changes were proposed in this pull request?
The
ThriftServerTabdisplays a FINISHED state when the operation finishes execution, but quite often it still takes a lot of time to fetch the results. OperationState has state CLOSED for when after the iterator is closed. This PR add CLOSED state to ExecutionState, and override theclose()in SparkExecuteStatementOperation, SparkGetColumnsOperation, SparkGetSchemasOperation and SparkGetTablesOperation.How was this patch tested?
manual tests
Thread.sleep(10000)before SparkExecuteStatementOperation.scala#L112ThriftServerTab: