Skip to content

[SPARK-20966][WEB-UI][SQL]Table data is not sorted by startTime time desc, time is not formatted and redundant code in JDBC/ODBC Server page.#18186

Closed
guoxiaolongzte wants to merge 37 commits intoapache:masterfrom
guoxiaolongzte:SPARK-20966
Closed

[SPARK-20966][WEB-UI][SQL]Table data is not sorted by startTime time desc, time is not formatted and redundant code in JDBC/ODBC Server page.#18186
guoxiaolongzte wants to merge 37 commits intoapache:masterfrom
guoxiaolongzte:SPARK-20966

Conversation

@guoxiaolongzte
Copy link
Copy Markdown

What changes were proposed in this pull request?

  1. Question 1 : Table data is not sorted by startTime time desc in JDBC/ODBC Server page.

fix before :
2

fix after :
21

  1. Question 2 : time is not formatted in JDBC/ODBC Server page.

fix before :
1

fix after :
11

  1. Question 3 : Redundant code in the ThriftServerSessionPage.scala.
    The function of 'generateSessionStatsTable' has not been used

How was this patch tested?

manual tests

Please review http://spark.apache.org/contributing.html before opening a pull request.

郭小龙 10207633 added 30 commits March 31, 2017 21:57
@guoxiaolongzte
Copy link
Copy Markdown
Author

@srowen @ajbozarth @jerryshao Help to review the code, thanks.

@guoxiaolongzte guoxiaolongzte changed the title [SPARK-20966]Table data is not sorted by startTime time desc, time is not formatted and redundant code in JDBC/ODBC Server page. [SPARK-20966][WEB-UI][SQL]Table data is not sorted by startTime time desc, time is not formatted and redundant code in JDBC/ODBC Server page. Jun 2, 2017
Copy link
Copy Markdown
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks ok, but I have questions regarding the reasoning behind the original implementations. @zsxwing you've worked in these files before, do you know why these original decision were made?

val headerRow = Seq("User", "JobID", "GroupID", "Start Time", "Finish Time", "Duration",
"Statement", "State", "Detail")
val dataRows = listener.getExecutionList
val dataRows = listener.getExecutionList.sortBy(_.startTimestamp).reverse
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was this sorted by before? Was there a reason it was that way? If there wasn't, I'm ok with this change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the ThriftServerSessionPage.scala, this sorted is order by startTimestamp desc.
But in the ThriftServerPage.scala, this sorted by before is default, this sorted is not order by startTimestamp desc.
So sort by time in reverse order is in line with spark UI style.

}

/** Generate stats of batch sessions of the thrift server program */
private def generateSessionStatsTable(): Seq[Node] = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell this has never been used, is this something that should be used rather than deleted? I have't worked in the ThiftSever UI much so I'm not sure myself.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the whole spark project, I did not find this method where it was used. When I remove this method to recompile the package test, I did not find any problems with this ThiftSever ui. SoI think can remove this method because it is redundant code. Thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a method like this in ThriftServerPage.scala above. I guess we can remove it too. It's dead code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, It's dead code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srowen
No, This method of generateSessionStatsTable in ThriftServerPage.scala is used. It is not dead code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1

@srowen
This method implements the functions shown in the figure

@guoxiaolongzte
Copy link
Copy Markdown
Author

@zsxwing
Help to review the code, thanks.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 3, 2017

Test build #3774 has finished for PR 18186 at commit 4c3dbeb.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 4, 2017

Test build #3776 has finished for PR 18186 at commit 4c3dbeb.

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

@srowen
Copy link
Copy Markdown
Member

srowen commented Jun 7, 2017

Ping @guoxiaolongzte

@guoxiaolongzte
Copy link
Copy Markdown
Author

@srowen
Is there anything you need to modify, sir?

@srowen
Copy link
Copy Markdown
Member

srowen commented Jun 7, 2017

You saw my comment at #18186 (comment) above right?

@guoxiaolongzte
Copy link
Copy Markdown
Author

@srowen
I have replied to your review, thank you.

@srowen
Copy link
Copy Markdown
Member

srowen commented Jun 7, 2017

Merged to master

@asfgit asfgit closed this in 0ca69c4 Jun 7, 2017
@guoxiaolongzte guoxiaolongzte deleted the SPARK-20966 branch June 12, 2017 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants