Skip to content

Conversation

@07ARB
Copy link
Contributor

@07ARB 07ARB commented Oct 25, 2019

What changes were proposed in this pull request?

Adding tooltip to SQL tab for better usability.

Why are the changes needed?

There are a few common points of confusion in the UI that could be clarified with tooltips. We
should add tooltips to explain.

Does this PR introduce any user-facing change?

yes.
Screenshot 2019-10-24 at 12 17 44 AM

How was this patch tested?

Manual test.

@07ARB
Copy link
Contributor Author

07ARB commented Oct 25, 2019

@dongjoon-hyun , @shahidki31 , @srowen (All the comments given in this PR #26216(closed) i have fixed and raised new PR).

@shahidki31
Copy link
Contributor

Jenkins, test this please

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.

I don't think this adds enough to justify. There just isn't much to explain about these.
(PS normally you push more commits to your previous branch rather than make a new one and PR, so as not to fork the review)

@07ARB
Copy link
Contributor Author

07ARB commented Oct 25, 2019

I don't think this adds enough to justify. There just isn't much to explain about these.
(PS normally you push more commits to your previous branch rather than make a new one and PR, so as not to fork the review)
Thank you @srowen , from next time onward i will take care of it.

@07ARB 07ARB requested a review from srowen October 25, 2019 19:02
@srowen
Copy link
Member

srowen commented Oct 25, 2019

If we can't come up with meaningful tooltips, why add them? I don't know if it is necessary here.

@07ARB
Copy link
Contributor Author

07ARB commented Oct 25, 2019

If we can't come up with meaningful tooltips, why add them? I don't know if it is necessary here.

ok then i will remove it but i shaw in Executors page they are displaying tooltips as header name,please check this image
Screenshot 2019-10-26 at 12 45 07 AM

@srowen
Copy link
Member

srowen commented Oct 25, 2019

Yep I think those tooltips are not useful and should be removed.

@07ARB
Copy link
Contributor Author

07ARB commented Oct 25, 2019

Yep I think those tooltips are not useful and should be removed.

ok, i will remove the tooltips.

@srowen , i think this 3 is ok, please check
Screenshot 2019-10-26 at 12 59 32 AM

and others i have removed.

    ### What changes were proposed in this pull request?
    Adding tooltip to SQL tab for better usability.
    https://issues.apache.org/jira/browse/SPARK-29453

    ### Why are the changes needed?
    There are a few common points of confusion in the UI that could be clarified with tooltips. We should add tooltips to explain.

    ### Does this PR introduce any user-facing change?
    yes.

    ### How was this patch tested?
    1) Unit tests (written unit test cases to verify changes).
    2) Manual test.

    Authored-by: Ankit Raj Boudh <ankitrajboudh@gmail.com>

fixed review comments
@07ARB
Copy link
Contributor Author

07ARB commented Oct 25, 2019

@dongjoon-hyun ,@srowen, I have fixed all the comments please check it. I think now every think is ok.

@07ARB 07ARB requested a review from dongjoon-hyun October 26, 2019 09:04
07ARB added 2 commits October 27, 2019 07:25
Tooltips not required for each row review comments fixed.
@07ARB
Copy link
Contributor Author

07ARB commented Oct 27, 2019

@shahidki31 and @srowen now I think fixed is ok?

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.

I just don't think this adds enough to justify more tooltips popping up. It isn't required to have every item with a tooltip.

@07ARB
Copy link
Contributor Author

07ARB commented Oct 27, 2019

@srowen, if headerTooltip is empty then no tooltips popup will display.

@07ARB 07ARB requested a review from srowen October 28, 2019 12:44
@srowen
Copy link
Member

srowen commented Oct 28, 2019

To be clear, I would not merge this.

@07ARB
Copy link
Contributor Author

07ARB commented Oct 28, 2019

Ok then I need add check point, if header is empty then no need to add tooltips, then it's ok?

@07ARB
Copy link
Contributor Author

07ARB commented Oct 28, 2019

#26259 (comment)
That's OK @srowen, you just help me to correct the code I will do it.

@srowen srowen closed this Nov 6, 2019
@07ARB
Copy link
Contributor Author

07ARB commented Nov 6, 2019

@srowen , so this issue not require to fix ? , (if yes then i will commit in jira not require to fix, so that reporter will cancel that jira)

@srowen
Copy link
Member

srowen commented Nov 6, 2019

As I say here and elsewhere, I don't think most of these columns have anything meaningful to say in a tooltop. Duration, maybe, if it's not clear what the end and start times are. But no I do not think we need most of what's being proposed here.

@07ARB
Copy link
Contributor Author

07ARB commented Nov 6, 2019

ok then for Duration i will add the tooltip for other no need to add that is ok @srowen ?

@srowen
Copy link
Member

srowen commented Nov 6, 2019

As long as it's not saying "the time taken to X", maybe so. You'd have to figure out how to correctly describe the start and end time it's measuring.

@07ARB
Copy link
Contributor Author

07ARB commented Nov 6, 2019

ok i will check this point (how to correctly describe the start and end time it's measuring.)

@07ARB
Copy link
Contributor Author

07ARB commented Nov 9, 2019

@srowen , this is how we are calculating duration
val duration = executionUIData.completionTime.map(_.getTime())
.getOrElse(currentTime) - submissionTime

and i referred this PR [SPARK-29019][WebUI] Improve tooltip JDBC/ODBC Server tab #25723

Tooltips for "Duration": "Difference between sql query execution finish time and submission time".

If this suggestion ok then i will submit PR for this

@07ARB
Copy link
Contributor Author

07ARB commented Nov 9, 2019

@srowen , please review this PR also if both is ok then i will start working on other issue.
#26263

@srowen
Copy link
Member

srowen commented Nov 10, 2019

I don't think that's quite accurate. It's either the time until the finish time, or the current time if still executing. How about more like "Time from query submission to completion (or if still executing, time since submission)". You/we might also review other similar tooltips about duration to see if we can make the wording more standard, but something like that.

@07ARB
Copy link
Contributor Author

07ARB commented Nov 11, 2019

#26259 (comment) @srowen , sure i will check other places also.

@07ARB
Copy link
Contributor Author

07ARB commented Nov 12, 2019

@srowen, I checked the code and found Total 5 pages of Spark showing Duration field inside table , Please check all the screenshot :

  1. Job page :
    Screenshot 2019-11-12 at 11 14 57 PM

  2. Stage page :
    Screenshot 2019-11-12 at 10 47 38 PM

Screenshot 2019-11-12 at 10 48 00 PM

  1. SQL page :

Screenshot 2019-11-12 at 10 49 02 PM

  1. JDBC/ODBC Server :

Screenshot 2019-11-12 at 10 49 26 PM

  1. Master Spark UI :
    Screenshot 2019-11-12 at 11 04 13 PM

Note : Active Driver and Completed Driver under master spark ui page also "Duration field is there."

@07ARB
Copy link
Contributor Author

07ARB commented Nov 12, 2019

@srowen , I think based on feature "Duration" field tooltips will change.
I will form tooltips (Only for "Duration" field) for above mention screenshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants