Skip to content

Conversation

@guoxiaolongzte
Copy link

@guoxiaolongzte guoxiaolongzte commented Sep 22, 2017

What changes were proposed in this pull request?

The 'job ids' list style needs to be changed in the SQL page. There are two reasons:

  1. If a job id is a line, there are a lot of job ids, then the table row height will be high. As shown below:
    3

  2. should be consistent with the 'JDBC / ODBC Server' page style, I am in this way to modify the style. As shown below:
    2

My changes are as follows:
6

5

How was this patch tested?

manual tests

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

@guoxiaolongzte
Copy link
Author

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

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

How about just space-separated? the braces all run together kind of overwhelm the digits

@guoxiaolongzte
Copy link
Author

I have fixed it.
7

@srowen
Copy link
Member

srowen commented Sep 22, 2017

Why braces? just seems like noise

@guoxiaolongzte
Copy link
Author

Do you want to get rid of braces?

Then the JDBC / ODBC Server page also needs to remove the braces. But I think there is a better distinction between brackets This is the job id.

@srowen
Copy link
Member

srowen commented Sep 22, 2017

I see, you're saying the display of a bunch of job IDs is like "[1] [2] [3]" elsewhere? consistency is the most important thing IMHO. I'd just match whatever a similar column does elsewhere.

@guoxiaolongzte
Copy link
Author

In JDBC / ODBC Server page.There are no other places yet.

@guoxiaolongzte
Copy link
Author

Do you agree with my PR or against my PR?

@srowen
Copy link
Member

srowen commented Sep 22, 2017

I agree with making things consistent. The question is, I guess, what the JDBC/ODBC page does. You can match that.

@guoxiaolongzte
Copy link
Author

In JDBC / ODBC Server page, 'job ids' are separated by braces, as shown below:
8

@HyukjinKwon
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Sep 23, 2017

Test build #82103 has finished for PR 19320 at commit 5cb6ea4.

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

@srowen
Copy link
Member

srowen commented Sep 23, 2017

Merged to master

@asfgit asfgit closed this in 3920af7 Sep 23, 2017
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