[SPARK-27468][Core][WEBUI] BlockUpdate replication event shouldn't overwrite storage level description in the UI #24398

shahidki31 · 2019-04-18T06:53:03Z

What changes were proposed in this pull request?

Test steps to reproduce this:

bin/spark-shell local-cluster[2,1,1024]

scala> import org.apache.spark.storage.StorageLevel
scala> val rdd = sc.parallelize(1 to 10, 1).persist(StorageLevel.MEMORY_ONLY_2)
scala> rdd.count

Events generated are shown like below

event: SparkListenerBlockUpdated(BlockUpdatedInfo(BlockManagerId(1, 10.8.132.160, 65473, None),rdd_0_0,StorageLevel(memory, deserialized, 2 replicas),56,0))
event: SparkListenerBlockUpdated(BlockUpdatedInfo(BlockManagerId(0, 10.8.132.160, 65474, None),rdd_0_0,StorageLevel(memory, deserialized, 1 replicas),56,0))

But in the UI, in the storage tab it displays in the description like,
"Memory Deserialized 1x Replicated", even though we have given replication as 2.

The root cause is that, the replication block update events will have replication factor 1. Hence in the AppStatusListener class, we overwrite whatever event comes later. If the replication event comes later, then we update replication factor as 1.

In the PR, I am fixing from the AppStatusListener class side, as we need to detect if the event is replication or not. Else we need to update the rdd store.

How was this patch tested?

Added UT and Manually tested.

Before patch:

After patch:

SparkQA · 2019-04-18T06:56:07Z

Test build #104692 has finished for PR 24398 at commit 86d109d.

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

shahidki31 · 2019-04-18T07:02:25Z

Jenkins, retest this please

SparkQA · 2019-04-18T11:53:32Z

Test build #104694 has finished for PR 24398 at commit 8d3c32e.

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

shahidki31 · 2019-04-18T11:57:34Z

cc @vanzin @srowen @zsxwing, kindly review

srowen · 2019-04-18T13:26:22Z

core/src/main/scala/org/apache/spark/status/LiveEntity.scala

  var storageLevel: String = weakIntern(info.storageLevel.description)
  var memoryUsed = 0L
  var diskUsed = 0L
+  var storageInfo: StorageLevel = new StorageLevel()


I don't know this part well, but is it redundant with storageLevel above?

The above was just a string representation of storage level. from StorageInfo we can get individual parameters including replication.

I see, but should we not just replace the field above with this richer object? or should this not use info.storageLevel as the initial value? maybe not, just jumped out at me as a question

Yes. we can initialize storageInfo = info.storageLevel. But I'm not sure we can get rid of storageLevel, as there is a public method which sets the value. updated the code.

srowen · 2019-04-18T13:28:09Z

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

+
      if (updatedStorageLevel.isDefined) {
-        rdd.setStorageLevel(updatedStorageLevel.get)
+        // Replicated block update events will have `storageLevel.replication=1`.


Is this a bug itself?

Needs more check, including impacts. Currently the fix is from UI side.

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

SparkQA · 2019-04-18T17:27:23Z

Test build #104708 has finished for PR 24398 at commit fbcc0c7.

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

shahidki31 · 2019-04-18T22:12:10Z

Retest this please.

SparkQA · 2019-04-19T00:23:33Z

Test build #104727 has finished for PR 24398 at commit fbcc0c7.

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

felixcheung · 2019-05-05T03:12:00Z

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

+        // Default value of  `storageInfo.replication = 1` and hence if
+        // `storeLevel.replication = 2`, the replicated events won't overwrite in the store.
+        val storageInfo = rdd.storageInfo
+        val isReplicatedBlockUpdateEvent = storageLevel.replication < storageInfo.replication &&


check if (storageLevel.isValid) before accessing storageLevel.*?

Hi, This line checks the storageLevel is valid or not.

spark/core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

Lines 916 to 920 in d9bcacf

val updatedStorageLevel = if (storageLevel.isValid) {

Some(storageLevel.description)

} else {

None

}

If not valid, then the updatedStorageLevel will be None. So, it won't come to this line (L-928).
Thanks

vanzin · 2019-05-06T18:33:44Z

After reading more of the storage code lately, I wonder if this code shouldn't just report the original storage level always. i.g., LiveRDD shouldn't have a writable storageLevel field at all, and instead the UI should always use the storage level from the respective RDDInfo.

shahidki31 · 2019-05-07T06:49:07Z

Thanks @vanzin . I have updated the code.

SparkQA · 2019-05-07T07:05:01Z

Test build #105198 has finished for PR 24398 at commit a22cd68.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

shahidki31 · 2019-05-07T07:30:05Z

Retest this please

SparkQA · 2019-05-07T09:47:21Z

Test build #105205 has finished for PR 24398 at commit a22cd68.

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

shahidki31 · 2019-05-07T10:20:07Z

Test results after the updated code,

 bin/spark-shell local-cluster[2,1,1024]

scala> import org.apache.spark.storage.StorageLevel
scala> val rdd = sc.parallelize(1 to 10, 1).persist(StorageLevel.MEMORY_ONLY_2)
scala> rdd.count

vanzin · 2019-05-07T17:42:57Z

So now the RDD storage level is what the user requested, which is fine. But what about the per-partition storage level? With your change it's just the same as the RDD level. Right thing to do would be to look at the behavior in Spark 2.2 and see how per-partition storage levels worked (unless someone remembers without looking at the code). You may have to propagate the block update's storage level to the partition.

vanzin · 2019-05-07T18:08:36Z

Yes, as I thought, in 2.2 the partition storage level comes from the block update:
https://github.com/apache/spark/blob/branch-2.2/core/src/main/scala/org/apache/spark/storage/StorageUtils.scala#L235

shahidki31 · 2019-05-07T18:59:28Z

@vanzin Yes. The behavior seems different compared to the 2.2 branch. I will update the PR.

vanzin · 2019-09-12T21:10:20Z

I created #25779 with a more complete fix for this, so closing this one.

[SPARK-27468]Storage Level" in "RDD Storage Page" is not correct

86d109d

scalastyle

8d3c32e

srowen reviewed Apr 18, 2019

View reviewed changes

adress comment

fbcc0c7

felixcheung reviewed May 5, 2019

View reviewed changes

address comments

a22cd68

shahidki31 force-pushed the SPARK-27468 branch from 8609fbe to a22cd68 Compare May 7, 2019 06:51

dongjoon-hyun added the SPARK CORE label Jun 14, 2019

vanzin closed this Sep 12, 2019

	val updatedStorageLevel = if (storageLevel.isValid) {
	Some(storageLevel.description)
	} else {
	None
	}

[SPARK-27468][Core][WEBUI] BlockUpdate replication event shouldn't overwrite storage level description in the UI #24398

[SPARK-27468][Core][WEBUI] BlockUpdate replication event shouldn't overwrite storage level description in the UI #24398

Uh oh!

Conversation

shahidki31 commented Apr 18, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 18, 2019

Uh oh!

shahidki31 commented Apr 18, 2019

Uh oh!

SparkQA commented Apr 18, 2019

Uh oh!

shahidki31 commented Apr 18, 2019

Uh oh!

srowen Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

shahidki31 Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

shahidki31 Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

srowen Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

shahidki31 Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Apr 18, 2019

Uh oh!

shahidki31 commented Apr 18, 2019

Uh oh!

SparkQA commented Apr 19, 2019

Uh oh!

felixcheung May 5, 2019

Choose a reason for hiding this comment

Uh oh!

shahidki31 May 5, 2019

Choose a reason for hiding this comment

Uh oh!

vanzin commented May 6, 2019

Uh oh!

shahidki31 commented May 7, 2019

Uh oh!

SparkQA commented May 7, 2019

Uh oh!

shahidki31 commented May 7, 2019

Uh oh!

SparkQA commented May 7, 2019

Uh oh!

shahidki31 commented May 7, 2019

Uh oh!

vanzin commented May 7, 2019

Uh oh!

vanzin commented May 7, 2019

Uh oh!

shahidki31 commented May 7, 2019

Uh oh!

vanzin commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shahidki31 Apr 18, 2019 •

edited

Loading