-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-2411] Add a history-not-found page to standalone Master #1336
Conversation
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16429/ |
Jenkins test this please |
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16433/ |
Jenkins test this please |
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16466/ |
Jenkins test this please |
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
Please don't merge this yet. @aarondav made a comment about the wording offline that I want to address. |
Provided the tests pass (they should) this is ready from my side. |
QA tests have started for PR 1336. This patch merges cleanly. |
QA results for PR 1336: |
All automated tests passed. |
val content = | ||
<div class="row-fluid"> | ||
<div class="span12" style="font-size:14px;font-weight:bold"> | ||
No event logs were found for this application. To enable event logging, please set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you also mention the setting for the event log directory? Otherwise they might think they can just set enabled
and it will auto-magically work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it kind of does auto-magically work, but sure I'll add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, but in most environments users will have to add an HDFS directory there that is shared... right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the code actually creates the directory for you, so the user doesn't need to add it herself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that if the are joining an existing cluster, they'll need to figure out what HDFS or FS path to set this too such that it's consistent with the path set by the person running the history server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the standalone Master, not the history server. Here the event log directory for each application is shipped to the master as part of the application description, so the Master knows where to find the logs automatically.
(For history server, however, the user should explicitly set the path in all cases as you describe)
LGTM with one small comment. |
I have updated the screenshot. Anything else? |
Looks good, thanks! |
Hey actually - could this differentiate between the case where the event log was not enabled (and give a message like "Application did not enable event logging") and a case where the event log was expected but could not be read (and give a message like "Error reading event log"). Bonus points if you can encode the error message in the second case. |
With this commit, the HistoryNotFoundPage behaves differently for each of the following scenarios: (1) User did not enable event logging -> point them to the configs (2) Event logs are not found for some reason -> tell them that (3) Exception is thrown in replaying -> show them the stack trace
QA tests have started for PR 1336. This patch merges cleanly. |
QA results for PR 1336: |
I have updated the screenshots again. Anything else? |
} else { | ||
logWarning("Application %s (%s) has no valid logs: %s".format(appName, app.id, eventLogDir)) | ||
|
||
if (eventLogPaths.isEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens here if eventLogDir
doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See L670. If eventLogDir
doesn't exist, we never even reach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If you mean the directory does not exist on the filesystem itself, then it does go into this case, and the page displays view (2) in the PR message)
Okay cool - I'm merging tihs. |
**Problem.** Right now, if you click on an application after it has finished, it simply refreshes the page if there are no event logs for the application. This is not super intuitive especially because event logging is not enabled by default. We should direct the user to enable this if they attempt to view a SparkUI after the fact without event logs. **Fix.** The new page conveys different messages in each of the following scenarios: (1) Application did not enable event logging, (2) Event logs are not found in the specified directory, and (3) Exception is thrown while replaying the logs Here are screenshots of what the page looks like in each of the above scenarios: (1) <img src="https://issues.apache.org/jira/secure/attachment/12656204/Event%20logging%20not%20enabled.png" width="75%"> (2) <img src="https://issues.apache.org/jira/secure/attachment/12656203/Application%20history%20not%20found.png"> (3) <img src="https://issues.apache.org/jira/secure/attachment/12656202/Application%20history%20load%20error.png" width="95%"> Author: Andrew Or <[email protected]> Closes apache#1336 from andrewor14/master-link and squashes the following commits: 2f06206 [Andrew Or] Merge branch 'master' of github.com:apache/spark into master-link 97cddc0 [Andrew Or] Add different severity levels 832b687 [Andrew Or] Mention spark.eventLog.dir in error message 51980c3 [Andrew Or] Merge branch 'master' of github.com:apache/spark into master-link ded208c [Andrew Or] Merge branch 'master' of github.com:apache/spark into master-link 89d6405 [Andrew Or] Reword message e7df7ed [Andrew Or] Add a history not found page to standalone Master
Problem. Right now, if you click on an application after it has finished, it simply refreshes the page if there are no event logs for the application. This is not super intuitive especially because event logging is not enabled by default. We should direct the user to enable this if they attempt to view a SparkUI after the fact without event logs.
Fix. The new page conveys different messages in each of the following scenarios:
(1) Application did not enable event logging,
(2) Event logs are not found in the specified directory, and
(3) Exception is thrown while replaying the logs
Here are screenshots of what the page looks like in each of the above scenarios:
(1)
(2)
(3)