[SPARK-22805][CORE] Use StorageLevel aliases in event logs #19992

superbobry · 2017-12-15T17:33:44Z

What changes were proposed in this pull request?

The format of event logs uses a redundant representation for storage
levels, for instance, StorageLevel.DISK_ONLY is represented as

{"Use Disk":true,"Use Memory":false,"Deserialized":false,"Replication":1}

which is 64 bytes more. This commit changes the event log representation
of the StorageLevel to predefined constants: NONE, DISK_ONLY, etc. The
change is fully backwards compatible.

How was this patch tested?

core unit tests.

The format of event logs uses redundant representation for storage levels, for instance StorageLevel.DISK_ONLY is represented as {"Use Disk":true,"Use Memory":false,"Deserialized":false,"Replication":1} which is 64 bytes more. This commit changes the event log representation of the StorageLevel to predefined constants: NONE, DISK_ONLY, etc. The change is fully backward compatibly, because * StorageLevel constructor is private, meaning that existing event logs can only contain these predefined levels; * The JsonProtocol supports reading both the old format and the new one.

superbobry · 2017-12-15T17:34:23Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

-    ("Deserialized" -> storageLevel.deserialized) ~
-    ("Replication" -> storageLevel.replication)
+  def storageLevelToJson(storageLevel: StorageLevel): JValue = storageLevel match {
+    case StorageLevel.NONE => "NONE"


Maybe we can change StorageLevel.toString to do this or add another method e.g. StorageLevel.name.

Agree, that would be more robust.

srowen

@andrewor14 I think you touched this last, and a long time ago. Just pinging you in case you have an opinion.

srowen · 2017-12-15T18:01:00Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

-    ("Deserialized" -> storageLevel.deserialized) ~
-    ("Replication" -> storageLevel.replication)
+  def storageLevelToJson(storageLevel: StorageLevel): JValue = storageLevel match {
+    case StorageLevel.NONE => "NONE"


Agree, that would be more robust.

srowen · 2017-12-15T18:30:17Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

-    val replication = (json \ "Replication").extract[Int]
-    StorageLevel(useDisk, useMemory, deserialized, replication)
+  def storageLevelFromJson(json: JValue): StorageLevel = json match {
+    case _: JString => StorageLevel.fromString(json.extract[String])


It's probably worth some comments about why there are two read paths

srowen · 2017-12-15T18:30:48Z

core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala

      |      "Port": 300
      |    },
      |    "Block ID": "rdd_0_0",
-      |    "Storage Level": {


We'll also want to retain some tests of the old format to ensure it's still read. Maybe there are outside of the diff and I'm not seeing them.

Added a to/from JSON test for a custom StorageLevel.

I guess I mean, doesn't this no longer test whether it can read the verbose, old style format? like this test does here and the ones above, that are being removed?

These strings are used with testEvent which assert that the serialized representation matches the one given in the string literal. See https://github.com/criteo-forks/spark/blob/7869e63a569a6fb6725996084f0c5c55fc130ac8/core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala#L457

I share sean's concern, I don't think your response is addressing it. You added one test on L151 makes sure that some event which is not predefined still works. But you don't have a test making sure that the old, verbose string can still be parsed (or is it somewhere else?)

probably this is indirectly covered by HistoryServerSuite but a more direrct test would be better

I've added a test ensuring all predefine storage levels can be read from the legacy format.

Sidenote: I've also noticed that the legacy format incorrectly handled the predefined StorageLevel.OFF_HEAP and an fact any other custom storage level with useOffHeap = true. It looks like a bug to me, wdyt?

yup, I completely agree that off heap is not respected in the json format. can you file a bug? I think its still relevant even after this goes in, for custom levels

Note that the previous commit contained a bug -- user-defined storage levels caused an exception in JsonProtocol.

vanzin · 2017-12-16T00:38:42Z

core/src/main/scala/org/apache/spark/storage/StorageLevel.scala

  override def hashCode(): Int = toInt * 41 + replication

+  /** Name of the storage level if it is predefined or [[None]] otherwise. */
+  def name: Option[String] = this match {


fromString below has basically the opposite of this. How about storing the mapping in a Seq[(StorageLevel, String)] and using that in both methods? e.g. here it would be:

knownLevels.collect { case (level, name) if level == this => name }.headOption

And pretty similar code in fromString.

This sounds good, will do. A slightly unrelated point: I feel that the name fromString somehow implies that it's the opposite of toString. What do you think about renaming it to fromName now that we have name?

What do you think about renaming

It's a public method so renaming means breaking compatibility.

vanzin · 2017-12-16T00:41:19Z

ok to test

SparkQA · 2017-12-16T00:44:08Z

Test build #84984 has finished for PR 19992 at commit 58e93bb.

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

SparkQA · 2017-12-16T12:16:07Z

Test build #85006 has finished for PR 19992 at commit 13fe385.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

superbobry · 2017-12-16T23:22:47Z

Minor update: I've simulated #18162 on one of our 80G event logs and (unless there is a bug in the filtering code) the log shrank to 157M. The effect of this patch was almost negligible, it brought the size down to 155M. It is unclear for now if this pattern generalizes to other workloads. See JIRA ticket for details.

SparkQA · 2017-12-16T23:26:14Z

Test build #85012 has finished for PR 19992 at commit e171f03.

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

SparkQA · 2017-12-17T13:29:02Z

Test build #85019 has finished for PR 19992 at commit 81b980f.

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

superbobry · 2017-12-18T20:01:18Z

Can someone have a look at the tests, please? I can't see the failure (and in theory, the change should not affect SparkR).

srowen · 2017-12-18T20:21:35Z

Ignore the error, it's being fixed separately.

SparkQA · 2017-12-19T18:27:44Z

Test build #4016 has finished for PR 19992 at commit 81b980f.

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

vanzin · 2017-12-19T19:49:31Z

core/src/main/scala/org/apache/spark/storage/StorageLevel.scala


+  /** Name of the storage level if it is predefined or `None` otherwise. */
+  def name: Option[String] = StorageLevel.PREDEFINED
+    .collectFirst { case (storageLevel, name) if storageLevel == this => name }


If the method body needs multiple lines it's better to wrap it with { }. Same thing below.

vanzin · 2017-12-19T23:06:30Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

-    ("Deserialized" -> storageLevel.deserialized) ~
-    ("Replication" -> storageLevel.replication)
-  }
+  def storageLevelToJson(storageLevel: StorageLevel): JValue =


Sorry, missed it. I've decided not to add braces to storageLevelFromJson because it seems to look OK with the toplevel match.

SparkQA · 2017-12-20T01:42:27Z

Test build #85135 has finished for PR 19992 at commit 59ed873.

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

jiangxb1987 · 2017-12-25T15:32:00Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

-    ("Replication" -> storageLevel.replication)
-  }
+  def storageLevelToJson(storageLevel: StorageLevel): JValue =
+    storageLevel.name match {


nit:

storageLevel.name.getOrElse(...)

Done. Sadly, in this case, getOrElse requires explicit type.

Actually, after seeing the compilation error, I recall why I went with a match instead of getOrElse -- the former does not require an explicit conversion to JString.

jiangxb1987 · 2017-12-25T15:34:41Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

+      val replication = (json \ "Replication").extract[Int]
+      StorageLevel(useDisk, useMemory, deserialized, replication)
+    case _ =>
+      throw new IllegalArgumentException(json.toString)


nit:

throw new IllegalArgumentException(s"Invalid storage level from json: $json.")

Done. I've changed the message to match the one in accumValueFromJson.

SparkQA · 2017-12-26T10:12:04Z

Test build #85402 has finished for PR 19992 at commit 1fc6e75.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2017-12-26T13:14:16Z

LGTM

SparkQA · 2017-12-26T16:10:04Z

Test build #85403 has finished for PR 19992 at commit b1e6f5f.

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

SparkQA · 2018-01-01T13:24:32Z

Test build #85579 has finished for PR 19992 at commit 9fbfe40.

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

SparkQA · 2018-01-01T17:12:55Z

Test build #85580 has finished for PR 19992 at commit cb1fe6a.

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

squito · 2018-01-05T05:38:25Z

change is fine, but from discussion on the jira I'm unclear if this is really worth it -- gain seems pretty small after the other fix in 2.3.

AmplabJenkins · 2018-01-18T17:30:36Z

Can one of the admins verify this patch?

superbobry · 2018-01-20T19:10:09Z

@squito I think it's fine to just close the PR/JIRA issue.

squito · 2018-01-22T20:14:55Z

thanks for looking into this @superbobry -- can you actually close this yourself? we can't directly close it (there is a way but its more complicated)

superbobry commented Dec 15, 2017

View reviewed changes

srowen requested changes Dec 15, 2017

View reviewed changes

Addressed review comments

58e93bb

Note that the previous commit contained a bug -- user-defined storage levels caused an exception in JsonProtocol.

vanzin reviewed Dec 16, 2017

View reviewed changes

Added StorageLevel.PREDEFINED to reduce boilerplate in name/fromString

13fe385

Fixed ScalaDoc

e171f03

Fixed a typo in a comment

81b980f

vanzin reviewed Dec 19, 2017

View reviewed changes

Added braces to StorageLevel.{name,fromString}

59ed873

vanzin reviewed Dec 19, 2017

View reviewed changes

More braces

e71f713

jiangxb1987 reviewed Dec 25, 2017

View reviewed changes

Addressed more review comments

b1e6f5f

superbobry force-pushed the compact-storage-level branch from 1fc6e75 to b1e6f5f Compare December 26, 2017 12:28

Added a test explicitly ensuring backward compatbility of StorageLevel

cb1fe6a

superbobry force-pushed the compact-storage-level branch from 9fbfe40 to cb1fe6a Compare January 1, 2018 13:32

superbobry closed this Jan 22, 2018

Willymontaz deleted the compact-storage-level branch April 2, 2019 15:06

[SPARK-22805][CORE] Use StorageLevel aliases in event logs #19992

[SPARK-22805][CORE] Use StorageLevel aliases in event logs #19992

Uh oh!

Conversation

superbobry commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen left a comment

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!

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!

vanzin commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

superbobry commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 17, 2017

Uh oh!

superbobry commented Dec 18, 2017

Uh oh!

srowen commented Dec 18, 2017

Uh oh!

SparkQA commented Dec 19, 2017

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!

SparkQA commented Dec 20, 2017

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!

SparkQA commented Dec 26, 2017

superbobry commented Dec 15, 2017 •

edited

Loading

superbobry commented Dec 16, 2017 •

edited

Loading