Skip to content
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-6796][Streaming][WebUI] Add "Active Batches" and "Completed Batches" lists to StreamingPage #5434

Closed
wants to merge 11 commits into from

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 9, 2015

This PR adds two lists, Active Batches and Completed Batches. Here is the screenshot:

batch_list

Due to SPARK-6766, I need to merge #5414 in my local machine to get the above screenshot.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 9, 2015

cc @tdas and @JoshRosen

@SparkQA
Copy link

SparkQA commented Apr 9, 2015

Test build #29922 has finished for PR 5434 at commit 8286a26.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@tdas
Copy link
Contributor

tdas commented Apr 9, 2015

This is great start. But we should be adding unit tests for this. Recently we have started writing unit tests for UI using selenium. Take a look at how this is written.

https://github.com/apache/spark/blob/master/streaming/src/test/scala/org/apache/spark/streaming/UISeleniumSuite.scala

Try adding unit test to that do some basic testing of this feature.

<h4 id="active">Active Batches ({runningBatches.size + waitingBatches.size})</h4> ++
new ActiveBatchTable(runningBatches, waitingBatches).toNodeSeq

val completedBatches = listener.retainedCompletedBatches.
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to the top of the function, i think more consistent.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 9, 2015

I added some tests to verify the title titles and column names.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 9, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Apr 10, 2015

Test build #29986 has finished for PR 5434 at commit 57f7663.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@tdas
Copy link
Contributor

tdas commented Apr 10, 2015

LGTM. Though, let me try running this locally and seeing the UI myself.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 11, 2015

Rebased on master.

@SparkQA
Copy link

SparkQA commented Apr 11, 2015

Test build #30060 has finished for PR 5434 at commit b248787.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

/**
* Return HTML for all rows of this table.
*/
protected def renderRows: Seq[Node] = Nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You could make this class abstract and leave this method undefined, then override it in the concrete classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Made BatchTableBase abstract.

@SparkQA
Copy link

SparkQA commented Apr 12, 2015

Test build #30125 has finished for PR 5434 at commit 86b5e7f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

val formattedProcessingTime = processingTime.map(UIUtils.formatDuration).getOrElse("-")

<td sorttable_customkey={batchTime.toString}>{formattedBatchTime}</td>
<td sorttable_customkey={eventCount.toString}>{eventCount.toString} events</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

One small nit that I noticed when i ran it myself. This says events where as else where in the UI it says records. Lets change it to events to keep things consistent.

@tdas
Copy link
Contributor

tdas commented Apr 13, 2015

Another points I noticed in the UI, in the Active Batches (XXX), the value is always = number of batches retained. Its looks like that only that many batches have completed whereas 1000 batches may have completed and we showing only 100 batches. I think we should show it as Active Batches (last XX out of YY) where YY is the sum of all completed batches.

@tdas
Copy link
Contributor

tdas commented Apr 13, 2015

Also, could you make the stats at the top of the page "Waiting batches" and "Processed batches" links to the corresponding sections? And the names should be consistent, so please rename them here, keep them as "Active Batches" and "Completed Batches".

@zsxwing
Copy link
Member Author

zsxwing commented Apr 13, 2015

@tdas I fixed as per your suggestion. I also made "startTime" more readable. Here is the new screenshot:

streaming

@SparkQA
Copy link

SparkQA commented Apr 13, 2015

Test build #30172 has finished for PR 5434 at commit 51b792e.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 13, 2015

Test build #30174 has finished for PR 5434 at commit be50fc6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@tdas
Copy link
Contributor

tdas commented Apr 14, 2015

I am merging this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants