Skip to content

Conversation

@JoshRosen
Copy link
Contributor

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

  private val appTable: UITable[ApplicationInfo] = {
    val t = new UITableBuilder[ApplicationInfo]()
    t.col("ID") (identity) withMarkup { app =>
      <a href={"app?appId=" + app.id}>{app.id}</a>
    }
    t.col("Name") { _.id }
    t.col("Cores") { _.coresGranted }
    t.sizeCol("Memory per Node") { _.desc.memoryPerSlave }
    t.dateCol("Submitted Time") { _.submitDate }
    t.col("User") { _.desc.user }
    t.col("State") { _.state.toString }
    t.durationCol("Duration") { _.duration }
    t.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.

TODO:

  • Update Stages page to use this
  • Tests of disabling sorting; audit current tables to figure out which columns should not be sortable.
  • Tooltip support.

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.
@SparkQA
Copy link

SparkQA commented Oct 20, 2014

QA tests have started for PR 2852 at commit c0aca09.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 20, 2014

QA tests have finished for PR 2852 at commit c0aca09.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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.

@SparkQA
Copy link

SparkQA commented Oct 21, 2014

QA tests have started for PR 2852 at commit 290f58b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 21, 2014

QA tests have finished for PR 2852 at commit 290f58b.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: import ordering

@kayousterhout
Copy link
Contributor

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.

@JoshRosen
Copy link
Contributor Author

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

an extra star?

@rxin
Copy link
Contributor

rxin commented Oct 23, 2014

Looks pretty cool overall. I made some comments, mostly around clarity of the code.

@JoshRosen JoshRosen changed the title [WIP] Add WebUITableBuilder to simplify table-building code [SPARK-4070] [WIP] Add WebUITableBuilder to simplify table-building code Oct 23, 2014
Make build() into a paren method.
- memCol -> sizeCol
- 4 spaces for parameter indentation
- move `case` statements
@SparkQA
Copy link

SparkQA commented Oct 23, 2014

QA tests have started for PR 2852 at commit b02c82a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 23, 2014

QA tests have finished for PR 2852 at commit b02c82a.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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.

This removes the messy "custom col" stuff and allows for a somewhat
more "declarative"-looking style when customizing markup rendering,
sorting, etc.
@JoshRosen
Copy link
Contributor Author

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:

  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.

@JoshRosen
Copy link
Contributor Author

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.

@SparkQA
Copy link

SparkQA commented Oct 24, 2014

QA tests have started for PR 2852 at commit 1975cd6.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 24, 2014

QA tests have finished for PR 2852 at commit 1975cd6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class UITableColumn[T, V](

@AmplabJenkins
Copy link

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.

@JoshRosen
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants