[SPARK-26726] Synchronize the amount of memory used by the broadcast variable to the UI display #23649

httfighter · 2019-01-25T09:13:04Z

…not synchronized to the UI display

What changes were proposed in this pull request?

The amount of memory used by the broadcast variable is not synchronized to the UI display.
I added the case for BroadcastBlockId and updated the memory usage.

How was this patch tested?

We can test this patch with unit tests.

…not synchronized to the UI display

dongjoon-hyun · 2019-01-25T19:33:21Z

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

    }
  }

+  def updateBroadcastBlock(event: SparkListenerBlockUpdated, broadcast: BroadcastBlockId): Unit = {


def -> private def?

Thank you for your review! I have submitted a new change.

dongjoon-hyun · 2019-01-25T19:39:08Z

Hi, @httfighter .
Since this is UI PR, could you attach the screenshots of BEFORE and AFTER?

vanzin · 2019-01-25T19:43:07Z

It's not really a UI PR. But it does need a unit test.

httfighter · 2019-01-28T07:55:46Z

@vanzin Thank you for your review! I have added the test case.

httfighter · 2019-01-28T08:20:18Z

@dongjoon-hyun Memory usage for the broadcast variable will not be updated synchronously in the UI before modification.

After the modification, the memory usage for the broadcast variable will be updated synchronously in the UI.

vanzin

Please use the PR title/description to explain the fix, not the problem.

vanzin · 2019-01-28T18:44:10Z

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

    }
  }

+  private def updateBroadcastBlock(event: SparkListenerBlockUpdated,


multi line args start on the next line.

vanzin · 2019-01-28T18:45:13Z

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

+    val memoryDelta = event.blockUpdatedInfo.memSize * (if (storageLevel.useMemory) 1 else -1)
+
+    // Function to apply a delta to a value, but ensure that it doesn't go negative.
+    def newValue(old: Long, delta: Long): Long = math.max(0, old + delta)


Already exists (addDeltaToValue).

vanzin · 2019-01-28T18:50:12Z

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

+
+    val maybeExec = liveExecutors.get(executorId)
+    maybeExec.foreach { exec =>
+      if (exec.hasMemoryInfo) {


This block exists in a very similar form in two other places. Feels like time to have a helper method.

httfighter · 2019-01-29T06:29:06Z

@vanzin Thanks for your advice. I have updated the code.

vanzin

Your PR title still explains the problem, not the fix.

vanzin · 2019-01-29T19:51:52Z

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

+
+  }
+
+  // update executor memory and disk usage info


This just repeats the method name. Remove.

vanzin · 2019-01-29T19:57:18Z

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

+      storageLevel: StorageLevel,
+      memoryDelta: Long,
+      diskDelta: Long,
+      OffHeapDelta: Option[Long],


Variable names start with lower case.

I'd also avoid the Option here. That causes extra allocations + boxing which this code should avoid.

In the broadcast update you could just repeat memoryDelta as the parameter, since that's what this boils down to...

vanzin · 2019-01-29T19:58:01Z

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

+      diskDelta: Long,
+      OffHeapDelta: Option[Long],
+      OnHeapDelta: Option[Long]): Unit = {
+    maybeExec.foreach { exec =>


Move the foreach to the caller. That avoids repeating the foreach, and you could wrap more logic that doesn't need to run when the executor is not found.

httfighter · 2019-01-30T06:27:13Z

@vanzin OK. Please help me review it again.

vanzin · 2019-01-30T18:32:58Z

ok to test

vanzin

Looks good pending tests.

vanzin · 2019-01-30T19:10:28Z

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

+    val diskDelta = event.blockUpdatedInfo.diskSize * (if (storageLevel.useDisk) 1 else -1)
+    val memoryDelta = event.blockUpdatedInfo.memSize * (if (storageLevel.useMemory) 1 else -1)
+
+    liveExecutors.get(executorId).foreach { exec =>


All the code above can be moved within the foreach.

@vanzin Yes! It is true. I have updated the code.

SparkQA · 2019-01-30T23:13:32Z

Test build #101904 has finished for PR 23649 at commit 221bf77.

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

SparkQA · 2019-01-31T06:36:46Z

Test build #101925 has finished for PR 23649 at commit 2024976.

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

vanzin · 2019-01-31T17:17:16Z

Merging to master / 2.4.

…variable to the UI display …not synchronized to the UI display ## What changes were proposed in this pull request? The amount of memory used by the broadcast variable is not synchronized to the UI display. I added the case for BroadcastBlockId and updated the memory usage. ## How was this patch tested? We can test this patch with unit tests. Closes #23649 from httfighter/SPARK-26726. Lead-authored-by: 韩田田00222924 <[email protected]> Co-authored-by: [email protected] <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]> (cherry picked from commit f4a17e9) Signed-off-by: Marcelo Vanzin <[email protected]>

vanzin · 2019-01-31T17:31:30Z

It was a clean backport to 2.3 and passed tests, so also merged to 2.3.

dongjoon-hyun · 2019-01-31T17:42:30Z

It's great! And, cc @maropu .

…variable to the UI display …not synchronized to the UI display ## What changes were proposed in this pull request? The amount of memory used by the broadcast variable is not synchronized to the UI display. I added the case for BroadcastBlockId and updated the memory usage. ## How was this patch tested? We can test this patch with unit tests. Closes apache#23649 from httfighter/SPARK-26726. Lead-authored-by: 韩田田00222924 <[email protected]> Co-authored-by: [email protected] <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]>

…variable to the UI display …not synchronized to the UI display ## What changes were proposed in this pull request? The amount of memory used by the broadcast variable is not synchronized to the UI display. I added the case for BroadcastBlockId and updated the memory usage. ## How was this patch tested? We can test this patch with unit tests. Closes apache#23649 from httfighter/SPARK-26726. Lead-authored-by: 韩田田00222924 <[email protected]> Co-authored-by: [email protected] <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]> (cherry picked from commit f4a17e9) Signed-off-by: Marcelo Vanzin <[email protected]>

[SPARK-26726] The amount of memory used by the broadcast variable is …

2459342

…not synchronized to the UI display

dongjoon-hyun reviewed Jan 25, 2019

View reviewed changes

add the test case

a51ea3e

vanzin reviewed Jan 28, 2019

View reviewed changes

Improve code modification

e9d0f18

Update the code

53cbc0b

vanzin reviewed Jan 29, 2019

View reviewed changes

httfighter changed the title ~~[SPARK-26726] The amount of memory used by the broadcast variable is …~~ [SPARK-26726] Synchronize the amount of memory used by the broadcast variable to the UI display Jan 30, 2019

httfighter added 2 commits January 30, 2019 14:21

Update the code

b4d6996

Update AppStatusListener.scala

221bf77

vanzin reviewed Jan 30, 2019

View reviewed changes

Update AppStatusListener.scala

2024976

asfgit closed this in f4a17e9 Jan 31, 2019

[SPARK-26726] Synchronize the amount of memory used by the broadcast variable to the UI display #23649

[SPARK-26726] Synchronize the amount of memory used by the broadcast variable to the UI display #23649

Uh oh!

Conversation

httfighter commented Jan 25, 2019 • 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!

dongjoon-hyun commented Jan 25, 2019

Uh oh!

vanzin commented Jan 25, 2019

Uh oh!

httfighter commented Jan 28, 2019

Uh oh!

httfighter commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanzin 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!

httfighter commented Jan 29, 2019

Uh oh!

vanzin 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!

httfighter commented Jan 30, 2019

Uh oh!

vanzin commented Jan 30, 2019

Uh oh!

vanzin 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!

SparkQA commented Jan 30, 2019

Uh oh!

SparkQA commented Jan 31, 2019

Uh oh!

vanzin commented Jan 31, 2019

Uh oh!

vanzin commented Jan 31, 2019

Uh oh!

dongjoon-hyun commented Jan 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

httfighter commented Jan 25, 2019 •

edited

Loading

httfighter commented Jan 28, 2019 •

edited

Loading