-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-2458] Make failed application log visible on History Server #3467
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
Conversation
|
In this PR, the sort order is (-endTime, -startTime) which means that Please see also the discuss in this previous PR about the need of this feature. #1558 |
|
Can one of the admins verify this patch? |
|
Looks great. I will take a detailed pass once the 1.2 release is out of the window. |
|
add to whitelist |
|
Test build #23875 has started for PR 3467 at commit
|
|
Test build #23875 has finished for PR 3467 at commit
|
|
Test FAILed. |
|
Test build #23892 has started for PR 3467 at commit
|
|
Test build #23892 has finished for PR 3467 at commit
|
|
Test PASSed. |
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.
requestedIncomplete == app.incomplete might be simpler
|
@ryan-williams Thank you for your review! |
|
Test build #23941 has started for PR 3467 at commit
|
|
Test build #23941 has finished for PR 3467 at commit
|
|
Test PASSed. |
|
Hey @tsudukim you will need to rebase to master because there have been changes to the HS that went in recently. Once you do that I will take a detailed pass through the PR. |
e69329b to
5454869
Compare
|
Test build #24413 has started for PR 3467 at commit
|
|
@andrewor14 Thank you for following this ticket. |
|
Test build #24413 has finished for PR 3467 at commit
|
|
Test FAILed. |
|
retest this please |
|
Test build #24596 has started for PR 3467 at commit
|
|
Test build #24596 has finished for PR 3467 at commit
|
|
Test PASSed. |
|
By the way I'm going to block this on #1222, since that one has been open for a while and I would like to get it merged first before @vanzin has to resolve more conflicts again. After that one goes in this PR will likely have to resolve non-trivial merge conflicts. I might hold back reviewing this PR until then. Just a heads up. |
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 does this mean? Which column does it sort by?
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.
About completed applications, they are sorted by endTime because they have proper endTime (almost unique).
About incomplete applications, they are sorted by startTime because they have the same invalid endTime(-1). As the first order is not effective, the second order is effective.
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 see
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.
space before if
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.
also might be good to check for info.endTime > 0 instead, here and other places
|
Hey @tsudukim this patch is pretty straightforward and it looks good. Unfortunately you will need to resolve a non-trivial merge conflict with another patch that I just merged. |
|
Thank you for your comments! I'm going to do it in a few days! |
Enabled HistoryServer to show incomplete applications. We can see the log for incomplete applications by clicking the bottom link.
Fixed "Incomplete" as capitalized in the web UI. Modified double negative variable name.
262dc52 to
f9ef854
Compare
|
Test build #24818 has started for PR 3467 at commit
|
|
Test build #24818 has finished for PR 3467 at commit
|
|
Test FAILed. |
|
Test build #24830 has started for PR 3467 at commit
|
|
Test build #24830 has finished for PR 3467 at commit
|
|
Test PASSed. |
|
resolved conflicts! |
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.
Return true when the application has completed.
Don't worry I'll fix the wording myself when I merge this
|
Alright LGTM I'm merging this into master thanks! |


Enabled HistoryServer to show incomplete applications.
We can see the log for incomplete applications by clicking the bottom link.