Skip to content

[SPARK-8171] [Web UI] Javascript based infinite scrolling for the log page - #10910

Closed
ajbozarth wants to merge 7 commits into
apache:masterfrom
ajbozarth:spark8171
Closed

[SPARK-8171] [Web UI] Javascript based infinite scrolling for the log page#10910
ajbozarth wants to merge 7 commits into
apache:masterfrom
ajbozarth:spark8171

Conversation

@ajbozarth

Copy link
Copy Markdown
Member

Updated the log page by replacing the current pagination with a javascript-based infinite scroll solution

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

I spent some time looking into ways to use javascript to load the content rather than use a page refresh and all the solutions I found required that the file has to be available on the web-server itself, which is very difficult to do with a continuously updating log file.

Instead I implemented a alteration to the current pagination that adds to the top (or bottom) of the log a page at a time rather than limiting you to read only a page at a time. By setting the scroll starting location at page load this simulates an infinite scroll even though the page is re-loading.

I also set the log to scroll to the bottom on initial page load.

Given this is inspired by but not exactly the original jira and does distinctly change the use of this page I would love to get peoples opinions on it's usability, look and feel, and if it's worth the change at all.

@JoshRosen

Copy link
Copy Markdown
Contributor

Screenshot? Please include them for all UI changes.

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

Sorry took me a bit longer to get them ready than usual.

Initial page load:
initial load

initial load scrolled to top:
initial top

after "load more" is clicked and page reloads:
after load more

if the full file is loaded:
top of file

when you scroll to the bottom after loading more and there is new log available:
load new

@SparkQA

Copy link
Copy Markdown

Test build #50050 has finished for PR 10910 at commit 4d7c433.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

Retest this please

@SparkQA

Copy link
Copy Markdown

Test build #50065 has finished for PR 10910 at commit 4d7c433.

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

@holdenk

Copy link
Copy Markdown
Contributor

So every time the user clicks "load more" it will request/fetch all of the log that has already been loaded again? (Just trying to understand the design).

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

When the user clicks load more the page refreshes and on the new page load it reads a larger chunk of the file.
The main implementation difference between the previous version and this is that when the user requests another page of the log it is added to the current log page(s) rather than just showing the new requested page alone.

@JoshRosen

Copy link
Copy Markdown
Contributor

This isn't quite what I had in mind when filing that JIRA; the UX that I envisioned was something where if a user clicks "load more" the Javascript requests only the new / additional portion log output from the backend then update the UI without reloading the page.

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

I had that in mind too, I just had this implementation in my head as an alternative and wanted to see what you thought. If you'd rather stick with what we have for now and wait until we figure out a js solution, then we can close this PR. I'll also spend just a bit more time trying to find a js solution to fetching the file data before moving to another task.

@JoshRosen

Copy link
Copy Markdown
Contributor

Yeah, let's close this for the time being and re-open when you have a new approach which is ready to be reviewed.

@ajbozarthajbozarth reopened this Jan 30, 2016
@ajbozarth

Copy link
Copy Markdown
MemberAuthor

Thanks @JoshRosen for rejecting my first version, making it did helped me find this new solution using js though.

Few questions:
Should we load the content of the log-data span and/or the logText content dynamically on initial page load or leave it hard-coded as is?
Would we rather have a proper infinite scroll where it detects when the user reaches the top or bottom rather than the buttons I've implemented? I prefer the buttons myself.

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

I'll post some new screen shots below but the look and feel is basically the same as the previous fake version.
I've changed the button text a bit and added an transient info alert when the user tries to load new and nothing is available. Note it doesn't replace the button but appears above it.

initialloadclick
bottomoflog
endoflog

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@ckadner this was what I was showing you if you want to take a look

@SparkQA

Copy link
Copy Markdown

Test build #50439 has finished for PR 10910 at commit 35e08c7.

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

@andrewor14

Copy link
Copy Markdown
Contributor

@sarutak

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@JoshRosen opinions on the updated version?

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

Just in case it fell through the cracks last week, what do people think of the new javascript-based log page?

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@tgravescs thanks for all the help on those other PRs, mind taking a look at this one?

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@sarutak@JoshRosen@tgravescs in case this slipped past you guys due to Spark Summit last week

@ajbozarthajbozarth changed the title [SPARK-8171] [Web UI] Simulated infinite scrolling using current pagination code in Spark log viewers[SPARK-8171] [Web UI] Javascript based infinite scrolling for the log pageFeb 26, 2016
@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@JoshRosen If you have time to take a second look at this, it's been sitting in limbo for a while, thanks

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@sarutak@JoshRosen@tgravescs I know you guys are busy getting ready for Spark Summit West, but I was wondering if you had an eta on looking at this? It's been open for a couple months now, thanks.

@sarutak

Copy link
Copy Markdown
Member

@ajbozarth I'll check it soon.


function tailLog() {
$(".log-content").scrollTop($(".log-content")[0].scrollHeight);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not good to repeat a query for the same dom-objects like $(.log-content).
Similar repetitions are in some places in this change.

@sarutak

Copy link
Copy Markdown
Member

@ajbozarth Sorry for having you wait. I've inspected quickly and left some comments.

@JoshRosen I think this feature itself is useful but is this the feature you had in mind?

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@sarutak thanks for the feedback, I'll look through and address them as I have time over the next couple days

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@sarutak I just went through your comments and responded and made fixes. I'm currently testing locally and will push my changes later today.

@SparkQA

Copy link
Copy Markdown

Test build #55251 has finished for PR 10910 at commit 0f49738.

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

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

@sarutak@JoshRosen What do you think of the latest changes?

@sarutak

Copy link
Copy Markdown
Member

@ajbozarth Thank you for the update. I'll check it next Sunday or Monday.

url: "/log" + baseParams + "&offset=" + offset + "&byteLength=" + moreByteLength,
success: function (data) {
var oldHeight = $(".log-content")[0].scrollHeight;
var newlineIndex = data.indexOf('\n')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: semicolon.

@sarutak

Copy link
Copy Markdown
Member

@ajbozarth I've left some comments about very trivial nits, otherwise LGTM.

Previous 0 B
</button>
}
val curLogLength = endByte - startByte;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:semicolon is not needed here.

@ajbozarth

Copy link
Copy Markdown
MemberAuthor

Fixed the semicolons, switching back and forth between js and scala when they're "optional" on both really messes with you. Thanks @sarutak

@SparkQA

Copy link
Copy Markdown

Test build #56098 has finished for PR 10910 at commit 293f1a4.

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

@sarutak

Copy link
Copy Markdown
Member

LGTM. If we have no other comments within 24 hours, I'll merge this.

@sarutak

Copy link
Copy Markdown
Member

Merging this into master. Thanks @ajbozarth !

@ajbozarth
ajbozarth deleted the spark8171 branch April 20, 2016 16:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ajbozarth@JoshRosen@SparkQA@holdenk@andrewor14@sarutak