SPARK-6541 - Sort executors by ID (numeric) #9165

jbonofre · 2015-10-19T06:14:47Z

"Force" the executor ID sort with Int.

srowen · 2015-10-19T08:02:50Z

core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala

Can this be sortBy(_.id)? and I assume it's already ints.

Yes, here, it's already an Int. Let me update for sortBy() instead of sortWith().

I can't use sortBy() due to toSeq (and so diverging implicit expansion for ordering). I keep sortWith() for now (or I have to change the toSeq statement).

Don't you just need a toList then? indeed a Seq may not be random-access and so hard to sort.

@srowen yes, it's what I thought. Let me update like this. Thanks (as usual ;))

@srowen actually Scala's List is a linked list. You may want toIndexedSeq. In any case, I think all of them will dump the data into an array for sorting (even ArrayBuffer), so it probably doesn't matter.

Fair enough, though I wonder then why the Seq didn't work. Not a big deal but I can't see a good semantic reason for sortBy to not work but sortWith to work.

yeah, that doesn't make sense to me either, I don't see any reason why toSeq wouldn't cut it -- seems to compile for me ...

andrewor14 · 2015-10-19T23:35:42Z

ok to test

SparkQA · 2015-10-20T01:46:48Z

Test build #43951 has finished for PR 9165 at commit 7b2a351.

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

srowen · 2015-10-20T07:41:43Z

I'm OK with this. Sorting executor IDs numerically seems like the intent anyway. I don't see a downside to sorting the display otherwise, other than the cost of sorting, but that's pretty trivial given the number of executors.

JoshRosen · 2015-10-20T21:41:17Z

core/src/main/scala/org/apache/spark/ui/jobs/ExecutorTable.scala

Are executor ids always guaranteed to be longs? If not, this could fail with a NumberFormatException.

Oh, it's just occurred to me that the driver is an executor with ID "driver" for example. @jbonofre did that not actually cause a problem?

Why not just use JS to trigger the sort in the frontend or to set a default sort order for the table? Are you worried about UI flickering?

@JoshRosen yes, it's an alternative. Let me update like this.

@jbonofre are you still working on this one?

I just checked the usage of sorttable (from kryogenix) used in Spark UI, and actually, we have to sort server side.
Basically, using sorttable js at page loading is wrong. Here's the extract for the sorttable FAQ:

"
Sorting the table when the page is loaded

Lots of people ask, "how do I make sorttable sort the table the first time the page is loaded?" The answer is: you don't. Sorttable is about changing the HTML that is served from your server without a page refresh. When the page is first served from the server, you have to incur the wait for it to be served anyway. So, if you want the table sorted when a page is first displayed, serve the table in sorted order. Tables often come out of a database; get the data from the database in a sorted order with an ORDER BY clause in your SQL. Any solution which involves you running sorttable as soon as the page loads (i.e., without user input) is a wrong solution.
"

So, according to @JoshRosen (good catch Josh ;)), executor ID is not necessary a long (for instance, "driver" executor). Anyway, let me check if I can sort (alphanumeric) on server side, matching the sorttable sort ordering.

Hm, that's not a great argument to me, as it means "you must implement sorting logic twice, always". It's not possible to trigger the JS after the table loads? in any event, it may not be so hard to sort both places, sure. But it's a weird sort: by number for things that can be numbers but then alphabetically otherwise. That has to be implemented two places. It may prove to be too much hassle, as in fact, executors aren't really numbers but only usually appear to be

I agree @srowen. I can call the sorttable sorting with something like onload. Let me try this.

I'm testing the usage of:

<script> { Unparsed { """ | window.onload = function() { | sorttable.innerSortFunction.apply(document.getElementById('executorid'), []) | }; """.stripMargin } } </script>

in the ExecutorTable, but also in the UIUtils generate table function (as used in stage view) to see if it works as expected.

It seems that the innerSortFunction function requires latest sorttable version. Let me try to upgrade.

jbonofre · 2015-11-01T20:10:08Z

Update with client side executor ID sorting. This PR answers the original request, but I think it makes sense to sort all executor tables the same way.

SparkQA · 2015-11-01T22:31:56Z

Test build #44766 has finished for PR 9165 at commit 24b2774.

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

jbonofre · 2015-11-05T14:26:30Z

Thoughts ?

srowen · 2015-11-05T14:32:02Z

core/src/main/resources/org/apache/spark/ui/static/sorttable.js

It's not a big deal, but stripping whitespace on lines just adds to the PR noise and possibility of merge conflicts. If you can disable this it's much preferred.

srowen · 2015-11-05T14:36:23Z

Hm, by the end here it seems like 90% of the changes are whitespace changes. It's enough compared to the small functional change that I think it's worth re-doing.

In the end how does this sort? numerically, but what about IDs like "driver"? does it sort on something like the leading prefix of digits?

Does it still need server-side sorting if so? I see that's added but it just sorts lexicographically.

jbonofre · 2015-11-05T16:28:08Z

Actually, I didn't change the file myself: I just upgraded to latest sorttable.js script (downloaded from the website). That explains the whitespace changes. I can try to clean it up.

The ordering is only client-side (no change on the server side), and it use numeric/alphanumeric sort. As you can see in the Jira, the user would like ordering (as when he clicks on table header) instead of the "default" one.

Thanks Sean !

srowen · 2015-11-05T16:31:15Z

Oh OK, then let's keep the whitespace changes, if they're part of the upstream code we're including. I wouldn't change it then.

There is still a sorting change in ApplicationPage.scala right?
What's the nature of the sort -- how do the following sort, just theoretically and out of curiosity:

driver
1
10
driver1
driver10
1driver
10driver

jbonofre · 2015-11-06T09:22:42Z

Let me prepare couple of screenshots to illustrate the sort order.

jbonofre · 2015-11-08T12:24:12Z

PR rebased/cleaned up/updated.

jbonofre · 2015-11-08T12:25:06Z

Here's the screenshot without the PR:

and here's the screenshot with the PR:

Note the default sorting.

srowen · 2015-11-08T13:48:18Z

Yeah I'm still curious where "driver" would sort for example? I think it's clear that sorting numbers as numbers is OK but I don't see how this handles non-numeric IDs. Any reasonable behavior seems fine; just want to make sure it works.

jbonofre · 2015-11-08T15:00:59Z

Yeah, I understand what you mean. Let me simulate the case and attach a screenshot. Thanks again Sean !

SparkQA · 2015-11-08T21:09:37Z

Test build #45313 has finished for PR 9165 at commit 1feedec.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * final class ShuffleSortDataFormat extends SortDataFormat<PackedRecordPointer, LongArray>\n * final class UnsafeSortDataFormat extends SortDataFormat<RecordPointerAndKeyPrefix, LongArray>\n * class ExecutionMemoryPool(\n * abstract class MemoryPool(lock: Object)\n * class StorageMemoryPool(lock: Object) extends MemoryPool(lock) with Logging\n * abstract class Writer extends BaseReadWrite\n * trait Writable\n * abstract class Reader[T] extends BaseReadWrite\n * trait Readable[T]\n * class LDAModel(JavaModelWrapper, JavaSaveable, Loader):\n * case class GetInternalRowField(child: Expression, ordinal: Int, dataType: DataType)\n * case class Expand(\n

jbonofre · 2015-11-09T06:58:51Z

PR rebased. Test failure seems related to flaky tests (Hive). Anyway, I'm trying on my box.

SparkQA · 2015-11-09T09:20:47Z

Test build #45348 has finished for PR 9165 at commit 602452a.

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

andrewor14 · 2015-11-09T21:44:47Z

core/src/main/resources/org/apache/spark/ui/static/sorttable.js

these are indented too much. Also please uses spaces where possible

andrewor14 · 2015-11-09T21:48:09Z

@jbonofre actually I do mind the whitespace changes. Elsewhere we try to use 2 space indentations in JavaScript instead of tabs. This PR currently makes the style inconsistent everywhere. Would you mind fixing these manually?

SparkQA · 2015-11-09T22:38:18Z

Test build #45408 has finished for PR 9165 at commit 94a36b9.

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

jbonofre · 2015-11-17T06:46:16Z

PR rebased

SparkQA · 2015-11-17T08:31:24Z

Test build #46064 has finished for PR 9165 at commit 9a79862.

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

srowen · 2015-11-17T12:12:55Z

core/src/main/resources/org/apache/spark/ui/static/sorttable.js

@jbonofre wanted to triple-check on this -- is this change intended?

My bad Sean, you are right: it's not intended (resulting upgrade to latest sorttable version). Let me fix that.

jbonofre · 2015-11-17T13:00:31Z

Rebase

SparkQA · 2015-11-17T13:14:19Z

Test build #2074 has finished for PR 9165 at commit 9a79862.

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

SparkQA · 2015-11-17T15:11:07Z

Test build #46085 has finished for PR 9165 at commit 5526031.

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

SparkQA · 2015-11-17T15:15:36Z

Test build #2078 has finished for PR 9165 at commit 5526031.

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

srowen · 2015-11-17T16:35:12Z

core/src/main/resources/org/apache/spark/ui/static/sorttable.js

Heh, github says this line changed but I don't see it. I'm worried it's a difference in some unprintable character that will throw this off. Any way to make sure this line really didn't change? copy and paste from the previous version or is that what you did?

Let me checkout from master and change just the innerStoreFunction.

jbonofre · 2015-11-17T16:45:09Z

Cleanup and rebase

SparkQA · 2015-11-17T18:51:32Z

Test build #46094 has finished for PR 9165 at commit 7c84c24.

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

srowen · 2015-11-17T18:56:34Z

All looks good now. And it still works ? :)

jbonofre · 2015-11-17T19:04:27Z

Lol ;) Let me prepare a screenshot ;)

jbonofre · 2015-11-17T20:11:07Z

Last screenshot showing that it still works ;)

jbonofre · 2015-11-17T20:12:04Z

Rebase

srowen · 2015-11-17T20:25:26Z

It's looking good. If there are no further comments I'll merge shortly.

jbonofre · 2015-11-17T20:27:42Z

Awesome, thanks Sean !

SparkQA · 2015-11-17T22:40:56Z

Test build #46104 has finished for PR 9165 at commit f6c5f3f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class ArrayConversion(elementConversion: JDBCConversion) extends JDBCConversion\n * abstract class JdbcDialect extends Serializable\n

"Force" the executor ID sort with Int. Author: Jean-Baptiste Onofré <jbonofre@apache.org> Closes #9165 from jbonofre/SPARK-6541. (cherry picked from commit e62820c) Signed-off-by: Sean Owen <sowen@cloudera.com>

srowen reviewed Oct 19, 2015
View reviewed changes

JoshRosen reviewed Oct 20, 2015
View reviewed changes

srowen reviewed Nov 5, 2015
View reviewed changes

jbonofre force-pushed the SPARK-6541 branch from 24b2774 to 1feedec Compare November 8, 2015 12:23

jbonofre force-pushed the SPARK-6541 branch from 1feedec to 602452a Compare November 9, 2015 06:57

jbonofre force-pushed the SPARK-6541 branch from 602452a to 94a36b9 Compare November 9, 2015 20:32

andrewor14 reviewed Nov 9, 2015
View reviewed changes

jbonofre force-pushed the SPARK-6541 branch from 941db75 to 9a79862 Compare November 17, 2015 06:46

srowen reviewed Nov 17, 2015
View reviewed changes

jbonofre force-pushed the SPARK-6541 branch from 3a6796f to 5526031 Compare November 17, 2015 13:00

srowen reviewed Nov 17, 2015
View reviewed changes

jbonofre force-pushed the SPARK-6541 branch from 03a02d6 to 7c84c24 Compare November 17, 2015 16:44

jbonofre added 6 commits November 17, 2015 21:11

SPARK-6541 - Sort executor by id

f7bbd4d

SPARK-6541 - Cleanup indentation and style

4446ffd

SPARK-6541 - Cleanup indentation in sorttable.js

d6154d9

Avoid unintended change in sorttable.js

078e438

SPARK-6541 - Avoid unintended changes in sorttable.js

04cb4ce

SPARK-6541 - Cleanup sorttable.js

f6c5f3f

jbonofre force-pushed the SPARK-6541 branch from 7c84c24 to f6c5f3f Compare November 17, 2015 20:11

asfgit closed this in e62820c Nov 18, 2015

SPARK-6541 - Sort executors by ID (numeric) #9165

SPARK-6541 - Sort executors by ID (numeric) #9165

Uh oh!

Conversation

jbonofre commented Oct 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Oct 19, 2015

Uh oh!

SparkQA commented Oct 20, 2015

Uh oh!

srowen commented Oct 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbonofre commented Nov 1, 2015

Uh oh!

SparkQA commented Nov 1, 2015

Uh oh!

jbonofre commented Nov 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Nov 5, 2015

Uh oh!

jbonofre commented Nov 5, 2015

Uh oh!

srowen commented Nov 5, 2015

Uh oh!

jbonofre commented Nov 6, 2015

Uh oh!

jbonofre commented Nov 8, 2015

Uh oh!

jbonofre commented Nov 8, 2015

Uh oh!

srowen commented Nov 8, 2015

Uh oh!

jbonofre commented Nov 8, 2015

Uh oh!

SparkQA commented Nov 8, 2015

Uh oh!

jbonofre commented Nov 9, 2015

Uh oh!

SparkQA commented Nov 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!