[SPARK-26726] Synchronize the amount of memory used by the broadcast variable to the UI display by httfighter · Pull Request #23649 · apache/spark

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

 }
 }

+ 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

 }
 }

+ private def updateBroadcastBlock(event: SparkListenerBlockUpdated,


multi line args start on the next line.

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

+ 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

+
+ 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

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


This just repeats the method name. Remove.

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

+ 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

+ 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

+ 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 <han.tiantian@zte.com.cn> Co-authored-by: han.tiantian@zte.com.cn <han.tiantian@zte.com.cn> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit f4a17e9) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

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. Closesapache#23649 from httfighter/SPARK-26726. Lead-authored-by: 韩田田00222924 <han.tiantian@zte.com.cn> Co-authored-by: han.tiantian@zte.com.cn <han.tiantian@zte.com.cn> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

…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. Closesapache#23649 from httfighter/SPARK-26726. Lead-authored-by: 韩田田00222924 <han.tiantian@zte.com.cn> Co-authored-by: han.tiantian@zte.com.cn <han.tiantian@zte.com.cn> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit f4a17e9) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

[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 displayJan 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 f4a17e9Jan 31, 2019

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