[SPARK-10541] [Web UI] Allow ApplicationHistoryProviders to provide their own text when there aren't any complete apps #15490
Conversation
|
Test build #66976 has finished for PR 15490 at commit
|
|
Jenkins, retest this please |
|
Test build #66985 has finished for PR 15490 at commit
|
|
@steveloughran Since you opened the JIRA do you mind taking a look? |
|
@ajbozarth I'm not a spark committer, I'm not capaclbe of getting stuff in. I did dd one comment to some of the code, otherwise nothing I have issues with. LGTM |
|
Thanks @steveloughran, I'm not seeing your code comment though. Also @srowen mind taking a look? |
| override def getEmptyListingHtml(): Seq[Node] = { | ||
| <p> | ||
| Did you specify the correct logging directory? Please verify your setting of | ||
| <span style="font-style:italic">spark.history.fs.logDirectory</span> |
There was a problem hiding this comment.
you couldn't actually include the current value here?
There was a problem hiding this comment.
That's a great idea, I'll update it
|
Oh, I see, new UI had meant I'd left the comment partially incomplete. Sorry. Just the one: printing out the actual log dir location. That makes it much easier to identify a configuration problem and allow anyone with access to the cluster FS just to take a quick ls of the directory to see what's up |
|
The dir is actually already displayed so I just added a few words to clarify that the current setting is displayed above.
|
|
Test build #67148 has finished for PR 15490 at commit
|
|
LGTM. Merging to master. |
…eir own text when there aren't any complete apps ## What changes were proposed in this pull request? I've added a method to `ApplicationHistoryProvider` that returns the html paragraph to display when there are no applications. This allows providers other than `FsHistoryProvider` to determine what is printed. The current hard coded text is now moved into `FsHistoryProvider` since it assumed that's what was being used before. I chose to make the function return html rather than text because the current text block had inline html in it and it allows a new implementation of `ApplicationHistoryProvider` more versatility. I did not see any security issues with this since injecting html here requires implementing `ApplicationHistoryProvider` and can't be done outside of code. ## How was this patch tested? Manual testing and dev/run-tests No visible changes to the UI Author: Alex Bozarth <[email protected]> Closes apache#15490 from ajbozarth/spark10541.
…eir own text when there aren't any complete apps ## What changes were proposed in this pull request? I've added a method to `ApplicationHistoryProvider` that returns the html paragraph to display when there are no applications. This allows providers other than `FsHistoryProvider` to determine what is printed. The current hard coded text is now moved into `FsHistoryProvider` since it assumed that's what was being used before. I chose to make the function return html rather than text because the current text block had inline html in it and it allows a new implementation of `ApplicationHistoryProvider` more versatility. I did not see any security issues with this since injecting html here requires implementing `ApplicationHistoryProvider` and can't be done outside of code. ## How was this patch tested? Manual testing and dev/run-tests No visible changes to the UI Author: Alex Bozarth <[email protected]> Closes apache#15490 from ajbozarth/spark10541.
4 participants
What changes were proposed in this pull request?
I've added a method to
ApplicationHistoryProviderthat returns the html paragraph to display when there are no applications. This allows providers other thanFsHistoryProviderto determine what is printed. The current hard coded text is now moved intoFsHistoryProvidersince it assumed that's what was being used before.I chose to make the function return html rather than text because the current text block had inline html in it and it allows a new implementation of
ApplicationHistoryProvidermore versatility. I did not see any security issues with this since injecting html here requires implementingApplicationHistoryProviderand can't be done outside of code.How was this patch tested?
Manual testing and dev/run-tests
No visible changes to the UI