Skip to content

Conversation

@uncleGen
Copy link
Contributor

What changes were proposed in this pull request?

before pr:
image

after pr:
image

How was this patch tested?

manual test

@uncleGen
Copy link
Contributor Author

cc @srowen

@SparkQA
Copy link

SparkQA commented May 15, 2019

Test build #105399 has finished for PR 24609 at commit 956f61f.

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

@uncleGen
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented May 15, 2019

Test build #105407 has finished for PR 24609 at commit 956f61f.

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

@SparkQA
Copy link

SparkQA commented May 16, 2019

Test build #105445 has finished for PR 24609 at commit 1f6c366.

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

</span> ++
<div class="stage-details collapsed">
<pre>{execution.description}<br></br>{execution.details}</pre>
<pre>{jobDescription}<br></br>{execution.details}</pre>
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems jobDescription trims the execution.description. But for long queries we shouldn't trim the query and need to display in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, do not understand what you mean. makeDescription returns HTML rendering of a job or stage description. It will try to parse the string as HTML and make sure that it only contains anchors with root-relative links. Otherwise, the whole string will rendered as a simple escaped text.

Copy link
Contributor

@shahidki31 shahidki31 May 17, 2019

Choose a reason for hiding this comment

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

In the details, before this change it used to show entire query if the query is large. But after the change, it seems not showing the entire sql query.

After change:
Screenshot from 2019-05-17 10-41-46

Before change:
Screenshot from 2019-05-17 10-41-28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shahidki31 Got it, let me confirm this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shahidki31 Could you please test the pr again? It is fixed in my local test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @uncleGen . Should we fix here too?

val description = paths.size match {
case 0 =>
s"Listing leaf files and directories 0 paths"
case 1 =>
s"Listing leaf files and directories for 1 path:<br/>${paths(0)}"
case s =>
s"Listing leaf files and directories for $s paths:<br/>${paths(0)}, ..."
}
sparkContext.setJobDescription(description)

Copy link
Member

Choose a reason for hiding this comment

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

@uncleGen I'd just look at this possible change too.

@SparkQA
Copy link

SparkQA commented May 17, 2019

Test build #105479 has finished for PR 24609 at commit ca1a1f7.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-27715][SQL] SQL query details in UI dose not show in correct format. [SPARK-27715][SQL][UI] SQL query details in UI dose not show in correct format. May 18, 2019
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.

That's safer, but, I wonder if we should just remove the <br/> from StreamExecution.getBatchDescriptionString. It doesn't seem essential, nor does it seem appropriate to add HTML there.

@gaborgsomogyi
Copy link
Contributor

s/UI dose not/UI does not

@uncleGen
Copy link
Contributor Author

@gaborgsomogyi Sorry, what do you mean?

@srowen
Copy link
Member

srowen commented Sep 25, 2019

(He's just suggesting you fix the typo in the title here)

@uncleGen
Copy link
Contributor Author

@srowen I removed the html tag from the StreamExecution.getBatchDescriptionString, and get correct format. Take a review please.

image

@uncleGen uncleGen requested a review from srowen September 25, 2019 12:14
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.

Yes, pending tests, that looks like a better fix.

@SparkQA
Copy link

SparkQA commented Sep 25, 2019

Test build #111346 has finished for PR 24609 at commit 47eb0f4.

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

@gaborgsomogyi
Copy link
Contributor

@uncleGen the title still contains the typo and it's not allowed to fix by reviewers. Can you please take care of it? Otherwise looks good.

@srowen srowen changed the title [SPARK-27715][SQL][UI] SQL query details in UI dose not show in correct format. [SPARK-27715][SQL][UI] SQL query details in UI does not show in correct format. Sep 26, 2019
@srowen
Copy link
Member

srowen commented Sep 26, 2019

(I fixed the title.)

@uncleGen
Copy link
Contributor Author

Thanks @srowen and @gaborgsomogyi

@srowen srowen closed this in 570525f Sep 27, 2019
@srowen
Copy link
Member

srowen commented Sep 27, 2019

Merged to master

@srowen
Copy link
Member

srowen commented Sep 27, 2019

(Currently having trouble with JIRA, even after updating to LDAP password; I'll update the issue when it works or else anyone else can resolve it)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants