-
Notifications
You must be signed in to change notification settings - Fork 598
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
[LIVY-344] [Web UI] Create Session Log Page #25
Conversation
Created a Session Log page for viewing the session log. Updated All Sessions Page and Session Page with links to the Log Page. Log Page exists for both Interactive Sessions and Batches.
Feedback Question:
|
@jerryshao Here's the next UI PR |
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.
Generally LGTM, just have two minor questions.
var type = pathArr.shift(); | ||
var id = pathArr.shift(); | ||
|
||
$.getJSON(location.origin + getLogPath(type, id), {from: 0, size: 100}, function(response) { |
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.
Based on the definition of from
and size
, here with your settings, it will always only show first 100 lines of log. I would instead use the default one. Please see the implementation of serializeLogs
in SessionServlet.scala
.
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 put 100 here as a placeholder until we address whether we want scrolling, as for why 100, it's the default value in the function you mentioned.
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.
My concern is actually from: 0
, by default it is -1 which has different meanings.
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.
Ah, that was so it always displays the first 100 lines, whereas using -1 it will display the last 100 lines. Either way I'll be addressing this based on my comment below since we'll need to read the whole log.
<li class="active"><a href="#">{sessionPage.name}</a></li> | ||
} | ||
case logPage: LogPage => { | ||
val sessionLink = if (logPage.sessionType == "Session") "/ui/session/" + logPage.id else "#" |
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.
does it mean that log will only be shown for interactive session?
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 the link used in the header. You'll notice when on the Log Page the Header will have the tabs: Sessions | Session # | Log
clicking Session #
will go to the session summary page. In the case of a Batch log the Header will look like Sessions | Batch # | Log
and click Batch #
will do nothing (href="#"
) since there's no Session Pages for Batches. I chose this option over linking back to the All Sessions Page, which was my other idea.
This would be good if it is not difficult to achieve.
Here based on you implementation, we will only show 100 lines of logs at most, do you think it is necessary to add a form scrolling? |
I'll separate out stderr/stdout/YARN tomorrow. And by scrolling I meant a form of infinite scrolling (which I implemented on Sparks Log Page), though after your comment about 100 lines and my suggestion on splitting up the log I've realized that if stderr or stdout was too long YARN Diag won't even load. So I will probably implement a way to load the whole log no matter length and have scrolling within each of the "sub-logs" |
Also based on everything I'm realizing in this conversation I may toy will how the logs are stored and accessed in order to better facilitate this page. (Without changing current API functionality of course) |
Pushed an update addressing the log length, after a little work I found that separating out the different logs is simple, but from a UI perspective splitting them up doesn't do much without optimizing the UI for multiple logs, therefore I'm splitting off a followup JIRA to deal with splitting up the logs so we can merge more of the UI now and move forward with a 0.4 release faster. |
opened up followup JIRA mentioned above: https://issues.cloudera.org/browse/LIVY-387 |
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
============================================
- Coverage 70.95% 70.76% -0.19%
- Complexity 778 781 +3
============================================
Files 97 97
Lines 5343 5364 +21
Branches 799 805 +6
============================================
+ Hits 3791 3796 +5
- Misses 1020 1033 +13
- Partials 532 535 +3
Continue to review full report at Codecov.
|
var from = fromOpt.getOrElse(-1) | ||
if (size < 0) { | ||
size = lines.length |
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.
Please be aware even we set size
to -1
, still we cannot get the whole logs, by default it will only cache the latest 200 lines (livy.cache-log.size)
.
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.
Oh, that code was merged after I wrote this (this has been sitting on my computer for a while waiting for the previous PRs to merge).
I feel like 200 is a really small default size for the logs, is there a reason that number was chosen @praveenkanamarlapudi ?
As for size=-1
, since it's used in js the conf value wouldn't be available so it will work well for what ever livy.cache-log.size
is set to.
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.
Yeah, it's fine for JS. I just want to mention relying on this to show the whole log is not a good idea.
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.
@ajbozarth we use livy a lot. We have seen memory issues due to logs(as it stored in memory). So, we set the default log size to 200 to resolve the memory issues.
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.
So I read through #14 to see exactly how livy.cache-log.size
is used and it's a line limit on each of the stdout and stderr sections of the log, not the log length itself. So the default max log size is actually (livy.cache-log.size * 2) + yarnDiagnostics.size + 3
(3 for each log header) and yarnDiagnostics
has no size limit that I can find. So I'm ok with that default size I think.
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.
thanks @praveenkanamarlapudi
LGTM based on my limited knowledge of JS :). |
I pushed a small update to the TODO comment, shouldn't affect travis results |
Thanks, will merge after travis finished. |
Travis failure are unrelated and passed before the update to the comment, is it ok to merge or should I re-trigger travis? |
Yes, please try again, thank you! |
1 similar comment
Yes, please try again, thank you! |
LIVY-344
Created a Session Log page for viewing the session log.
Updated All Sessions Page and Session Page with links to the Log Page.
Log Page exists for both Interactive Sessions and Batches.
Tested Manually, screenshots below