Skip to content

Conversation

@guoxiaolongzte
Copy link

@guoxiaolongzte guoxiaolongzte commented May 17, 2017

What changes were proposed in this pull request?

propose:

it provide links that jump to Running Queries,Completed Queries and Failed Queries.
it add (count) about Running Queries,Completed Queries and Failed Queries.
This is a small optimization in in the SQL web ui.

fix before:

before

fix after:

after

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
…nning Queries (" + listener.getRunningExecutions.size + ")"
@guoxiaolongzte
Copy link
Author

@SparkQA
please test it.

@SparkQA
Copy link

SparkQA commented May 26, 2017

Test build #3761 has finished for PR 18015 at commit 21e2c31.

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

@SparkQA
Copy link

SparkQA commented May 26, 2017

Test build #3764 has finished for PR 18015 at commit 21e2c31.

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

@guoxiaolongzte
Copy link
Author

@srowen Help to merge to master and modify the status about https://issues.apache.org/jira/browse/SPARK-20785.Thanks.

@guoxiaolongzte
Copy link
Author

@srowen
Help to merge to master and modify the status about https://issues.apache.org/jira/browse/SPARK-20785.Thanks.

@srowen
Copy link
Member

srowen commented May 31, 2017

I don't think those links are worth the vertical space they chew up

@guoxiaolongzte
Copy link
Author

guoxiaolongzte commented May 31, 2017

@srowen
I think the style exists to provide links that jump to Running Queries or Completed Queries because it may be well below the fold if there are many workers. Job page, stage page, application page, master page, worker page and other pages, but also this style.

@srowen
Copy link
Member

srowen commented May 31, 2017

Yes I understand that, but I don't think the reasoning is as compelling here, and the style is a bit different.

@guoxiaolongzte
Copy link
Author

@srowen
SQL ui is really different from other ui. In the follow-up version, I will strive to improve the function of SQL ui.

@guoxiaolongzte
Copy link
Author

@srowen
Sir, what about the PR need to be modified?

@guoxiaolongzte
Copy link
Author

@srowen
Help to merge to master , Thanks.

@srowen
Copy link
Member

srowen commented Jun 5, 2017

I don't support this but don't object of someone else merges

@guoxiaolongzte
Copy link
Author

@srowen
Sir, if so, I do not know how to deal with it.
But I really feel like this change, which makes spark SQL UI good. If you agree with this JIRA, I also plan to add this function to this SQL ui. I believe that spark SQL ui will be more powerful and easy to use for users. Thanks.

@guoxiaolongzte
Copy link
Author

@gatorsmile
Help to merge to master , Thanks.

@HyukjinKwon
Copy link
Member

Hi @jerryshao, do you maybe have a preference? To me, I don't have a strong preference but I think I am okay with this.

{listener.getFailedExecutions.size}
</li>
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the indention here correct? This seems a little weird to me.

Copy link
Member

Choose a reason for hiding this comment

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

Please follow the style in the other files of the package org.apache.spark.sql.execution.ui

@jerryshao
Copy link
Contributor

Jenkins, retest this please.

@jerryshao
Copy link
Contributor

Yes, I'm fine with it. @ajbozarth would you please take another look on this PR? Thanks.

@SparkQA
Copy link

SparkQA commented Sep 22, 2017

Test build #82064 has finished for PR 18015 at commit 21e2c31.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@guoxiaolongzte
Copy link
Author

This Jenkins error is not caused by this PR.

@HyukjinKwon
Copy link
Member

retest this please

@guoxiaolongzte
Copy link
Author

And failed, who submitted the code in question.

@SparkQA
Copy link

SparkQA commented Sep 22, 2017

Test build #82072 has finished for PR 18015 at commit 21e2c31.

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

@ajbozarth
Copy link
Member

Still LGTM

@guoxiaolongzte
Copy link
Author

@HyukjinKwon
Help merge to master.

@HyukjinKwon
Copy link
Member

I think @jerryshao is an active committer who knows this one better than me. Since he is here, let me leave it to him.

@jerryshao
Copy link
Contributor

There's still left comment not addressed.

@guoxiaolongzte
Copy link
Author

Sorry, I accidentally deleted the code branch. I'm going to close this PR. I created a new PR #19346, which was modified and created based on the latest code.
In the new PR I fix the indentation problem.
Thanks.

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.

7 participants