You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by JoshRosen <gi...@git.apache.org> on 2014/10/20 08:10:53 UTC

[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

GitHub user JoshRosen opened a pull request:

    https://github.com/apache/spark/pull/2852

    [WIP] Add WebUITableBuilder to simplify table-building code

    This work-in-progress commit illustrates a weekend hack project that I came up with for significantly simplifying the web UI's table rendering code.  See the huge block comment in `WebUITableBuilder` for more details.  This isn't ready to merge yet; I wanted to get some feedback before converting the rest of the table construction code to use this (I know that I should open a JIRA for this, too; I'll do it tomorrow).
    
    Essentially, this commit adds a small builder class for constructing objects that know how to render web UI tables.  This builder helps us to avoid several sources of errors / maintenance headaches, such as duplicate/boilerplate markup, inconsistent formatting of columns in different tables (e.g. durations or memory being displayed differently), separation of column names from column data values, etc.  This is best illustrated by some sample code; this new framework lets you write
    
    ```scala
      private val appTable: UITable[ApplicationInfo] = {
        val builder = new UITableBuilder[ApplicationInfo]()
        import builder._
        customCol("ID") { app =>
          <a href={"app?appId=" + app.id}>{app.id}</a>
        }
        col("Name") { _.id }
        intCol("Cores") { _.coresGranted }
        memCol("Memory per Node") { _.desc.memoryPerSlave }
        dateCol("Submitted Time") { _.submitDate }
        col("User") { _.desc.user }
        col("State") { _.state.toString }
        durationCol("Duration") { _.duration }
        build
      }
    ```
    
    to render the "applications" table in the standalone Master UI.  I find this significantly easier to understand and maintain than the old code.  For example, this makes it trivial to re-order columns.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/JoshRosen/spark webui-table-builder

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/2852.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2852
    
----
commit c0aca09d676ce750496451f3691c5f9e861103bd
Author: Josh Rosen <jo...@databricks.com>
Date:   2014-10-20T06:02:29Z

    Add WebUITableBuilder to clean up table building code.
    
    This significantly simplifies / abstracts the web UI's table construction
    code, which seems to account for the majority of the UI code.  I haven't
    converted all tables to use this yet; this commit just provides the basic
    framework and a few example usages in the master web UI.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59687732
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21907/consoleFull) for   PR 2852 at commit [`c0aca09`](https://github.com/apache/spark/commit/c0aca09d676ce750496451f3691c5f9e861103bd).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60410884
  
    Going to close this for now.  I've looked into it some more and there are a few corner-cases in the job info page that this doesn't handle super-cleanly.  Since this is a kind of leaky abstraction, it might be more confusing than what we have now; we can revisit this later.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen closed the pull request at:

    https://github.com/apache/spark/pull/2852


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59969443
  
    This looks awesome! Will it be hard to add classes to rows / columns (as is needed in #2867, for example) with this?  That's one thing that was pretty cumbersome with the current code, and I'm wondering if this will make things like that harder or easier.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19165764
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    + * To remove some of the boilerplate here, you can statically import the `col` methods; for example:
    + *
    + *    val myTable: UITable[MyRowDataType] = {
    + *      val builder = new UITableBuilder[MyRowDataType]()
    + *      import builder._
    + *      col("Name") { _.name }
    + *      [...]
    + *      build
    + *    }
    + *
    + * There are many other features, including support for arbitrary markup in custom column types;
    + * see the actual uses in the web UI code for more details.
    + *
    + * @param fixedWidth if true, all columns will be rendered with the same width
    + * @tparam T the type of the data items that will be used to render individual rows
    + */
    +private[spark] class UITableBuilder[T](fixedWidth: Boolean = false) {
    +  private val cols = mutable.Buffer[UITableColumn[T, _]]()
    +
    +  /**
    +   * Display a column with custom HTML markup.  The markup should only describe how to
    +   * render the contents of the TD tag, not the TD tag itself.
    +   */
    +  def customCol[V](
    +    name: String,
    --- End diff --
    
    I think this should be indented 4 spaces, not two (which is what we do elsewhere for function declarations) -- same for many of the other functions defined here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60324848
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22094/consoleFull) for   PR 2852 at commit [`b02c82a`](https://github.com/apache/spark/commit/b02c82a7ca6ecd75d65c6c945a1574fecfb22f76).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59891923
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21978/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59891599
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21978/consoleFull) for   PR 2852 at commit [`290f58b`](https://github.com/apache/spark/commit/290f58b47fd1a77b61f8ead753db534098224dbc).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60340080
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22107/consoleFull) for   PR 2852 at commit [`1975cd6`](https://github.com/apache/spark/commit/1975cd6364d6403ec2d7f885743020b55b10fea9).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300540
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala ---
    @@ -21,12 +21,28 @@ import javax.servlet.http.HttpServletRequest
     
     import scala.xml.Node
     
    -import org.apache.spark.ui.{WebUIPage, UIUtils}
    +import org.apache.spark.ui.{UITableBuilder, UITable, WebUIPage, UIUtils}
     
     private[spark] class HistoryPage(parent: HistoryServer) extends WebUIPage("") {
     
       private val pageSize = 20
     
    +  val appTable: UITable[ApplicationHistoryInfo] = {
    +    val builder = new UITableBuilder[ApplicationHistoryInfo]()
    +    import builder._
    +    customCol("App ID") { info =>
    +      val uiAddress = HistoryServer.UI_PATH_PREFIX + s"/${info.id}"
    +      <a href={uiAddress}>{info.id}</a>
    +    }
    +    col("App Name") { _.name }
    +    epochDateCol("Started") { _.startTime }
    +    epochDateCol("Completed") { _.endTime }
    +    durationCol("Duration") { info => info.endTime - info.startTime }
    +    col("Spark User") { _.sparkUser }
    +    epochDateCol("Last Updated") { _.lastUpdated }
    +    build
    --- End diff --
    
    as commented later, make sure you add parentheses to build()


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19165811
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    + * To remove some of the boilerplate here, you can statically import the `col` methods; for example:
    + *
    + *    val myTable: UITable[MyRowDataType] = {
    + *      val builder = new UITableBuilder[MyRowDataType]()
    + *      import builder._
    + *      col("Name") { _.name }
    + *      [...]
    + *      build
    + *    }
    + *
    + * There are many other features, including support for arbitrary markup in custom column types;
    + * see the actual uses in the web UI code for more details.
    + *
    + * @param fixedWidth if true, all columns will be rendered with the same width
    + * @tparam T the type of the data items that will be used to render individual rows
    + */
    +private[spark] class UITableBuilder[T](fixedWidth: Boolean = false) {
    +  private val cols = mutable.Buffer[UITableColumn[T, _]]()
    +
    +  /**
    +   * Display a column with custom HTML markup.  The markup should only describe how to
    +   * render the contents of the TD tag, not the TD tag itself.
    +   */
    +  def customCol[V](
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[T => String] = None)(renderer: T => Seq[Node]): UITableBuilder[T] = {
    +    val customColumn = new UITableColumn[T, T](name, null, sortable, sortKey, identity) {
    +      override def renderCellContents(row: T) = renderer(row)
    +    }
    +    cols.append(customColumn)
    +    this
    +  }
    +
    +  def col[V](
    +    name: String,
    +    formatter: V => String,
    +    sortable: Boolean = true,
    +    sortKey: Option[V => String] = None)(fieldExtractor: T => V): UITableBuilder[T] = {
    +    cols.append(UITableColumn(name, formatter, sortable, sortKey, fieldExtractor))
    +    this
    +  }
    +
    +  def col(
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[String => String] = None)(fieldExtractor: T => String): UITableBuilder[T] = {
    +    col[String](name, {x: String => x}, sortable, sortKey)(fieldExtractor)
    +  }
    +
    +  def intCol(
    +    name: String,
    +    formatter: Int => String = { x: Int => x.toString },
    +    sortable: Boolean = true)(fieldExtractor: T => Int): UITableBuilder[T] = {
    +    col[Int](name, formatter, sortable = sortable)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of memory sizes, in megabytes, as human-readable strings, such as "4.0 MB".
    +   */
    +  def memCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    --- End diff --
    
    Can this be sizeCol instead? It seems like a column describing data size on disk (which we need in a few places) should use the same format


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60343806
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22107/consoleFull) for   PR 2852 at commit [`1975cd6`](https://github.com/apache/spark/commit/1975cd6364d6403ec2d7f885743020b55b10fea9).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class UITableColumn[T, V](`



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60339925
  
    My latest commit cleans up the messy `customCol` and adds a builder-like pattern to the columns, too, so you have this nice, declarative-looking style for customizing columns' sorting / formatting / markup behavior:
    
    ```scala
      private val workerTable: UITable[WorkerInfo] = {
        val t = new UITableBuilder[WorkerInfo]()
        t.col("ID") (identity) withMarkup  { worker =>
          <a href={worker.webUiAddress}>{worker.id}</a>
        }
        t.col("Address") { worker => s"${worker.host}:${worker.port}"}
        t.col("State") { _.state.toString }
        t.col("Cores") { _.coresUsed } formatWith { c: Int => s"$c Used" }
        t.col("Memory") (identity) sortBy { worker =>
          s"${worker.memory}:${worker.memoryUsed}"
        } withMarkup { worker =>
          Text(Utils.megabytesToString(worker.memory)) ++
          Text(Utils.megabytesToString(worker.memoryUsed))
        }
        t.build()
      }
    ```
    
    I know that a lot of folks dislike the infix syntax; I'm a fan of it here for these `sortBy` -> `withMarkup` chains, but I'm pretty flexible on this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19301013
  
    --- Diff: streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingPage.scala ---
    @@ -75,37 +89,45 @@ private[ui] class StreamingPage(parent: StreamingTab)
         val receivedRecordDistributions = listener.receivedRecordsDistributions
         val lastBatchReceivedRecord = listener.lastReceivedBatchRecords
         val table = if (receivedRecordDistributions.size > 0) {
    -      val headerRow = Seq(
    -        "Receiver",
    -        "Status",
    -        "Location",
    -        "Records in last batch\n[" + formatDate(Calendar.getInstance().getTime()) + "]",
    -        "Minimum rate\n[records/sec]",
    -        "Median rate\n[records/sec]",
    -        "Maximum rate\n[records/sec]",
    -        "Last Error"
    -      )
    -      val dataRows = (0 until listener.numReceivers).map { receiverId =>
    -        val receiverInfo = listener.receiverInfo(receiverId)
    -        val receiverName = receiverInfo.map(_.name).getOrElse(s"Receiver-$receiverId")
    -        val receiverActive = receiverInfo.map { info =>
    -          if (info.active) "ACTIVE" else "INACTIVE"
    -        }.getOrElse(emptyCell)
    -        val receiverLocation = receiverInfo.map(_.location).getOrElse(emptyCell)
    -        val receiverLastBatchRecords = formatNumber(lastBatchReceivedRecord(receiverId))
    -        val receivedRecordStats = receivedRecordDistributions(receiverId).map { d =>
    -          d.getQuantiles(Seq(0.0, 0.5, 1.0)).map(r => formatNumber(r.toLong))
    -        }.getOrElse {
    -          Seq(emptyCell, emptyCell, emptyCell, emptyCell, emptyCell)
    +      val tableRenderer: UITable[(Int, Option[ReceiverInfo])] = {
    +        val builder = new UITableBuilder[(Int, Option[ReceiverInfo])]()
    +        import builder._
    +        col("Receiver") { case (receiverId, receiverInfo) =>
    +          receiverInfo.map(_.name).getOrElse(s"Receiver-$receiverId")
    +        }
    +        col("Status") { case (_, receiverInfo) =>
    +          receiverInfo.map { info => if (info.active) "ACTIVE" else "INACTIVE" }.getOrElse(empty)
    +        }
    +        col("Location") { case (_, receiverInfo) => receiverInfo.map(_.location).getOrElse(empty) }
    +        col("Records in last batch\n[" + formatDate(Calendar.getInstance().getTime()) + "]") {
    +          case (receiverId, _) => formatNumber(lastBatchReceivedRecord(receiverId))
             }
    -        val receiverLastError = listener.receiverInfo(receiverId).map { info =>
    -          val msg = s"${info.lastErrorMessage} - ${info.lastError}"
    -          if (msg.size > 100) msg.take(97) + "..." else msg
    -        }.getOrElse(emptyCell)
    -        Seq(receiverName, receiverActive, receiverLocation, receiverLastBatchRecords) ++
    -          receivedRecordStats ++ Seq(receiverLastError)
    +        col("Minimum rate\n[records/sec]") {
    +          case (receiverId, _) => receivedRecordDistributions(receiverId).map {
    --- End diff --
    
    i think the implicit style here is to put case on the previous line?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300512
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala ---
    @@ -21,12 +21,28 @@ import javax.servlet.http.HttpServletRequest
     
     import scala.xml.Node
     
    -import org.apache.spark.ui.{WebUIPage, UIUtils}
    +import org.apache.spark.ui.{UITableBuilder, UITable, WebUIPage, UIUtils}
     
     private[spark] class HistoryPage(parent: HistoryServer) extends WebUIPage("") {
     
       private val pageSize = 20
     
    +  val appTable: UITable[ApplicationHistoryInfo] = {
    +    val builder = new UITableBuilder[ApplicationHistoryInfo]()
    +    import builder._
    +    customCol("App ID") { info =>
    +      val uiAddress = HistoryServer.UI_PATH_PREFIX + s"/${info.id}"
    +      <a href={uiAddress}>{info.id}</a>
    +    }
    +    col("App Name") { _.name }
    --- End diff --
    
    I really like this change, but I find col slightly confusing here. For somebody who comes to the code base without understanding the details of UITableBuilder and the import up there, they can get confused on where col is coming from.
    
    Why don't we just remove the import builder._ and add builder to every line? If you feel this is too long, just use a shorter variable name like t. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60324752
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22094/consoleFull) for   PR 2852 at commit [`b02c82a`](https://github.com/apache/spark/commit/b02c82a7ca6ecd75d65c6c945a1574fecfb22f76).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60343809
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22107/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59687734
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21907/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60340078
  
    Oh, and to be clear, the full grammar is something like
    
    t.col(_column name_)(_function for extracting field_) withSort (_function for transforming extracted field into a sort key_) formatWith (_function for transforming extracted field for display in table cell_) [isSortable()]
    
    I'll add a comment to clarify this.
    
    There's only one table left to go (the main stages table), plus some testing, then I think this should be good to merge.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300117
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    --- End diff --
    
    an extra star?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60324852
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22094/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4070] [WIP] Add WebUITableBuilder to si...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19313715
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala ---
    @@ -21,12 +21,28 @@ import javax.servlet.http.HttpServletRequest
     
     import scala.xml.Node
     
    -import org.apache.spark.ui.{WebUIPage, UIUtils}
    +import org.apache.spark.ui.{UITableBuilder, UITable, WebUIPage, UIUtils}
     
     private[spark] class HistoryPage(parent: HistoryServer) extends WebUIPage("") {
     
       private val pageSize = 20
     
    +  val appTable: UITable[ApplicationHistoryInfo] = {
    +    val builder = new UITableBuilder[ApplicationHistoryInfo]()
    +    import builder._
    +    customCol("App ID") { info =>
    +      val uiAddress = HistoryServer.UI_PATH_PREFIX + s"/${info.id}"
    +      <a href={uiAddress}>{info.id}</a>
    +    }
    +    col("App Name") { _.name }
    --- End diff --
    
    Do you think that the Javadoc comment at the top of UITableBuilder mitigates this confusion?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300818
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    + * To remove some of the boilerplate here, you can statically import the `col` methods; for example:
    + *
    + *    val myTable: UITable[MyRowDataType] = {
    + *      val builder = new UITableBuilder[MyRowDataType]()
    + *      import builder._
    + *      col("Name") { _.name }
    + *      [...]
    + *      build
    + *    }
    + *
    + * There are many other features, including support for arbitrary markup in custom column types;
    + * see the actual uses in the web UI code for more details.
    + *
    + * @param fixedWidth if true, all columns will be rendered with the same width
    + * @tparam T the type of the data items that will be used to render individual rows
    + */
    +private[spark] class UITableBuilder[T](fixedWidth: Boolean = false) {
    +  private val cols = mutable.Buffer[UITableColumn[T, _]]()
    +
    +  /**
    +   * Display a column with custom HTML markup.  The markup should only describe how to
    +   * render the contents of the TD tag, not the TD tag itself.
    +   */
    +  def customCol[V](
    --- End diff --
    
    because this is a builder pattern, wouldn't it make more sense we call this addCustomCol, addCol, addIntCol...?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300885
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/storage/RDDPage.scala ---
    @@ -19,16 +19,47 @@ package org.apache.spark.ui.storage
     
     import javax.servlet.http.HttpServletRequest
     
    -import scala.xml.Node
    +import scala.xml.{Text, Node}
     
     import org.apache.spark.storage.{BlockId, BlockStatus, StorageStatus, StorageUtils}
    -import org.apache.spark.ui.{WebUIPage, UIUtils}
    +import org.apache.spark.ui.{UITableBuilder, UITable, WebUIPage, UIUtils}
     import org.apache.spark.util.Utils
     
     /** Page showing storage details for a given RDD */
     private[ui] class RDDPage(parent: StorageTab) extends WebUIPage("rdd") {
       private val listener = parent.listener
     
    +  private val workerTable: UITable[(Int, StorageStatus)] = {
    +    val builder = new UITableBuilder[(Int, StorageStatus)]()
    +    import builder._
    +    col("Host") { case (_, status) =>
    +      s"${status.blockManagerId.host}:${status.blockManagerId.port}"
    +    }
    +    def getMemUsed(x: (Int, StorageStatus)): String = x._2.memUsedByRdd(x._1).toString
    +    customCol(
    +      "Memory usage",
    +      sortKey = Some(getMemUsed)) { case (rddId, status) =>
    +        val used = Utils.bytesToString(status.memUsedByRdd(rddId))
    +        val remaining = Utils.bytesToString(status.memRemaining)
    +        Text(s"$used ($remaining Remaining)")
    +      }
    +    memCol("Disk Usage") { case (rddId, status) => status.diskUsedByRdd(rddId) }
    +    build
    +  }
    +
    +  val blockTable: UITable[(BlockId, BlockStatus, Seq[String])] = {
    --- End diff --
    
    can this one be private? (i haven't looked at the context so maybe you cannot do that)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60013108
  
    @kayousterhout In general, I think that this should simplify special formatting rules for rows / columns by allowing us to centralize that logic in a single place.  I could easily add hooks to let you apply custom per-row formatting rules based on user-defined conditions, such as highlighting "bad" rows in red.  I'm going to push some examples of this in a little bit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59891918
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21978/consoleFull) for   PR 2852 at commit [`290f58b`](https://github.com/apache/spark/commit/290f58b47fd1a77b61f8ead753db534098224dbc).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19165361
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    --- End diff --
    
    super nit: import ordering


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300234
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    + * To remove some of the boilerplate here, you can statically import the `col` methods; for example:
    + *
    + *    val myTable: UITable[MyRowDataType] = {
    + *      val builder = new UITableBuilder[MyRowDataType]()
    + *      import builder._
    + *      col("Name") { _.name }
    + *      [...]
    + *      build
    + *    }
    + *
    + * There are many other features, including support for arbitrary markup in custom column types;
    + * see the actual uses in the web UI code for more details.
    + *
    + * @param fixedWidth if true, all columns will be rendered with the same width
    + * @tparam T the type of the data items that will be used to render individual rows
    + */
    +private[spark] class UITableBuilder[T](fixedWidth: Boolean = false) {
    +  private val cols = mutable.Buffer[UITableColumn[T, _]]()
    +
    +  /**
    +   * Display a column with custom HTML markup.  The markup should only describe how to
    +   * render the contents of the TD tag, not the TD tag itself.
    +   */
    +  def customCol[V](
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[T => String] = None)(renderer: T => Seq[Node]): UITableBuilder[T] = {
    +    val customColumn = new UITableColumn[T, T](name, null, sortable, sortKey, identity) {
    +      override def renderCellContents(row: T) = renderer(row)
    +    }
    +    cols.append(customColumn)
    +    this
    +  }
    +
    +  def col[V](
    +    name: String,
    +    formatter: V => String,
    +    sortable: Boolean = true,
    +    sortKey: Option[V => String] = None)(fieldExtractor: T => V): UITableBuilder[T] = {
    +    cols.append(UITableColumn(name, formatter, sortable, sortKey, fieldExtractor))
    +    this
    +  }
    +
    +  def col(
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[String => String] = None)(fieldExtractor: T => String): UITableBuilder[T] = {
    +    col[String](name, {x: String => x}, sortable, sortKey)(fieldExtractor)
    +  }
    +
    +  def intCol(
    +    name: String,
    +    formatter: Int => String = { x: Int => x.toString },
    +    sortable: Boolean = true)(fieldExtractor: T => Int): UITableBuilder[T] = {
    +    col[Int](name, formatter, sortable = sortable)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of memory sizes, in megabytes, as human-readable strings, such as "4.0 MB".
    +   */
    +  def memCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](
    +      name,
    +      formatter = Utils.megabytesToString,
    +      sortKey = Some(x => x.toString))(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of dates as yyyy/MM/dd HH:mm:ss format.
    +   */
    +  def dateCol(name: String)(fieldExtractor: T => Date): UITableBuilder[T] = {
    +    col[Date](name, formatter = UIUtils.formatDate)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of dates as yyyy/MM/dd HH:mm:ss format.
    +   */
    +  def epochDateCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](name, formatter = UIUtils.formatDate)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of durations, in milliseconds, as human-readable strings, such as "12 s".
    +   */
    +  def durationCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](name, formatter = UIUtils.formatDuration, sortKey = Some(_.toString))(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of optional durations, in milliseconds, as human-readable strings,
    +   * such as "12 s".  If the duration is None, then '-' will be displayed.
    +   */
    +  def optDurationCol(name: String)(fieldExtractor: T => Option[Long]): UITableBuilder[T] = {
    +    col[Option[Long]](
    +      name,
    +      formatter = { _.map(UIUtils.formatDuration).getOrElse("-")},
    +      sortKey = Some(_.getOrElse("-").toString)
    +    )(fieldExtractor)
    +  }
    +
    +  def build: UITable[T] = {
    --- End diff --
    
    build should have parenthesis, and make sure you add that to everywhere you call it. otherwise it looks like we are just returning a variable called build


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-60294242
  
    Looks pretty cool overall. I made some comments, mostly around clarity of the code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2852#discussion_r19300207
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/UITables.scala ---
    @@ -0,0 +1,251 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.ui
    +
    +import java.util.Date
    +
    +import scala.collection.mutable
    +import scala.xml.{Text, Node}
    +
    +import org.apache.spark.util.Utils
    +
    +
    +/**
    + * Describes how to render a column of values in a web UI table.
    + *
    + * @param name the name / title of this column
    + * @param formatter function that formats values for display in the table
    + * @param sortable if false, this column will not be sortable
    + * @param sortKey optional function for sorting by a key other than `formatter(value)`
    + * @param fieldExtractor function for extracting this field's value from the table's row data type
    + * @tparam T the table's row data type
    + * @tparam V this column's value type
    + */
    +private case class UITableColumn[T, V](
    +  name: String,
    +  formatter: V => String,
    +  sortable: Boolean,
    +  sortKey: Option[V => String],
    +  fieldExtractor: T => V) {
    +
    +  /** Render the TD tag for this row */
    +  def renderCell(row: T): Seq[Node] =  {
    +    val data = fieldExtractor(row)
    +    val cellContents = renderCellContents(data)
    +    <td sorttable_customkey={sortKey.map(k => Text(k(data)))}>
    +      {cellContents}
    +    </td>
    +  }
    +
    +  /** Render the contents of the TD tag for this row.  The contents may be a string or HTML */
    +  def renderCellContents(data: V): Seq[Node] = {
    +    Text(formatter(data))
    +  }
    +}
    +
    +/**
    + * Describes how to render a table to display rows of type `T`.
    + * @param cols a sequence of UITableColumns that describe how each column should be rendered
    + * @param fixedWidth if true, all columns of this table will be displayed with the same width
    + * @tparam T the row data type
    + */
    +private[spark] class UITable[T] (cols: Seq[UITableColumn[T, _]], fixedWidth: Boolean) {
    +
    +  private val tableClass = if (fixedWidth) {
    +    UIUtils.TABLE_CLASS + " table-fixed"
    +  } else {
    +    UIUtils.TABLE_CLASS
    +  }
    +
    +  private val colWidthAttr = if (fixedWidth) Some(Text((100.toDouble / cols.size) + "%")) else None
    +
    +  private val headerRow: Seq[Node] = {
    +    val headers = cols.map(_.name)
    +    // if none of the headers have "\n" in them
    +    if (headers.forall(!_.contains("\n"))) {
    +      // represent header as simple text
    +      headers.map(h => <th width={colWidthAttr}>{h}</th>)
    +    } else {
    +      // represent header text as list while respecting "\n"
    +      headers.map { case h =>
    +        <th width={colWidthAttr}>
    +          <ul class="unstyled">
    +            { h.split("\n").map { case t => <li> {t} </li> } }
    +          </ul>
    +        </th>
    +      }
    +    }
    +  }
    +
    +  private def renderRow(row: T): Seq[Node] = {
    +    val tds = cols.map(_.renderCell(row))
    +    <tr>{ tds }</tr>
    +  }
    +
    +  /** Render the table with the given data */
    +  def render(data: Iterable[T]): Seq[Node] = {
    +    val rows = data.map(renderRow)
    +    <table class={tableClass}>
    +      <thead>{headerRow}</thead>
    +      <tbody>
    +        {rows}
    +      </tbody>
    +    </table>
    +  }
    +}
    +
    +/**
    + * Builder for constructing web UI tables.  This builder offers several advantages over constructing
    + * tables by hand using raw XML:
    + *
    + *  - All of the table's data and formatting logic can live in one place; the table headers and
    + *    rows aren't described in separate code.  This prevents several common errors, like changing
    + *    the ordering of two column headers but forgetting to re-order the corresponding TD tags.
    + *
    + *  - No repetition of code for type-specific display rules: common column types like "memory",
    + *    "duration", and "time" have convenience methods that implement the right formatting logic.
    + *
    + *  - Details of our specific markup are generally abstracted away.  For example, the markup for
    + *    setting a custom sort key on a column now lives in one place, rather than being repeated
    + *    in each table.
    + *
    + * The recommended way of using this class:
    + *
    + *  - Create a new builder that is parametrized by the type (`T`) of data that you want to render.
    + *    In many cases, there may be some record type like `WorkerInfo` that holds all of the
    + *    information needed to render a particular row.  If the data for each table row comes from
    + *    several objects, you can combine those objects into a tuple or case-class.
    + *
    + *  - Use the `col` methods to add columns to this builder.  The final argument of each `col` method
    + *    is a function that extracts the column's field from a row object of type `T`.  Columns are
    + *    displayed in the order that they are added to the builder.  For most columns, you can write
    + *    code like
    + *
    + *      builder.col("Id") { _.id }
    + *      builder.memCol("Memory" { _.memory }
    + *
    + *    Columns have additional options, such as controlling their sort keys; see the individual
    + *    methods' documentation for more details.
    + *
    + *  - Call `build` to construct an immutable object which can be used to render tables.
    + *   *
    + * To remove some of the boilerplate here, you can statically import the `col` methods; for example:
    + *
    + *    val myTable: UITable[MyRowDataType] = {
    + *      val builder = new UITableBuilder[MyRowDataType]()
    + *      import builder._
    + *      col("Name") { _.name }
    + *      [...]
    + *      build
    + *    }
    + *
    + * There are many other features, including support for arbitrary markup in custom column types;
    + * see the actual uses in the web UI code for more details.
    + *
    + * @param fixedWidth if true, all columns will be rendered with the same width
    + * @tparam T the type of the data items that will be used to render individual rows
    + */
    +private[spark] class UITableBuilder[T](fixedWidth: Boolean = false) {
    +  private val cols = mutable.Buffer[UITableColumn[T, _]]()
    +
    +  /**
    +   * Display a column with custom HTML markup.  The markup should only describe how to
    +   * render the contents of the TD tag, not the TD tag itself.
    +   */
    +  def customCol[V](
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[T => String] = None)(renderer: T => Seq[Node]): UITableBuilder[T] = {
    +    val customColumn = new UITableColumn[T, T](name, null, sortable, sortKey, identity) {
    +      override def renderCellContents(row: T) = renderer(row)
    +    }
    +    cols.append(customColumn)
    +    this
    +  }
    +
    +  def col[V](
    +    name: String,
    +    formatter: V => String,
    +    sortable: Boolean = true,
    +    sortKey: Option[V => String] = None)(fieldExtractor: T => V): UITableBuilder[T] = {
    +    cols.append(UITableColumn(name, formatter, sortable, sortKey, fieldExtractor))
    +    this
    +  }
    +
    +  def col(
    +    name: String,
    +    sortable: Boolean = true,
    +    sortKey: Option[String => String] = None)(fieldExtractor: T => String): UITableBuilder[T] = {
    +    col[String](name, {x: String => x}, sortable, sortKey)(fieldExtractor)
    +  }
    +
    +  def intCol(
    +    name: String,
    +    formatter: Int => String = { x: Int => x.toString },
    +    sortable: Boolean = true)(fieldExtractor: T => Int): UITableBuilder[T] = {
    +    col[Int](name, formatter, sortable = sortable)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of memory sizes, in megabytes, as human-readable strings, such as "4.0 MB".
    +   */
    +  def memCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](
    +      name,
    +      formatter = Utils.megabytesToString,
    +      sortKey = Some(x => x.toString))(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of dates as yyyy/MM/dd HH:mm:ss format.
    +   */
    +  def dateCol(name: String)(fieldExtractor: T => Date): UITableBuilder[T] = {
    +    col[Date](name, formatter = UIUtils.formatDate)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of dates as yyyy/MM/dd HH:mm:ss format.
    +   */
    +  def epochDateCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](name, formatter = UIUtils.formatDate)(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of durations, in milliseconds, as human-readable strings, such as "12 s".
    +   */
    +  def durationCol(name: String)(fieldExtractor: T => Long): UITableBuilder[T] = {
    +    col[Long](name, formatter = UIUtils.formatDuration, sortKey = Some(_.toString))(fieldExtractor)
    +  }
    +
    +  /**
    +   * Display a column of optional durations, in milliseconds, as human-readable strings,
    +   * such as "12 s".  If the duration is None, then '-' will be displayed.
    +   */
    +  def optDurationCol(name: String)(fieldExtractor: T => Option[Long]): UITableBuilder[T] = {
    +    col[Option[Long]](
    +      name,
    +      formatter = { _.map(UIUtils.formatDuration).getOrElse("-")},
    +      sortKey = Some(_.getOrElse("-").toString)
    +    )(fieldExtractor)
    +  }
    +
    +  def build: UITable[T] = {
    +    val immutableCols: Seq[UITableColumn[T, _]] = cols.toSeq
    +    new UITable[T](immutableCols, fixedWidth)
    +  }
    +}
    --- End diff --
    
    nit: add a new line


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP] Add WebUITableBuilder to simplify table-...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2852#issuecomment-59687664
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21907/consoleFull) for   PR 2852 at commit [`c0aca09`](https://github.com/apache/spark/commit/c0aca09d676ce750496451f3691c5f9e861103bd).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org