[SPARK-20386][Spark Core]modify the log info if the block exists on the slave already #17683

eatoncys · 2017-04-19T10:29:41Z

What changes were proposed in this pull request?

Modify the added memory size to memSize-originalMemSize if the block exists on the slave already
since if the block exists, the added memory size should be memSize-originalMemSize; if originalMemSize is bigger than memSize ,then the log info should be Removed memory, removed size should be originalMemSize-memSize

How was this patch tested?

Multiple runs on existing unit tests

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

srowen · 2017-04-19T10:31:01Z

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

+            blockId, blockManagerId.hostPort, Utils.bytesToString(memSize-originalMemSize),
+            Utils.bytesToString(_remainingMem)))
+        }
+        else{


Lots of style problems in this change. Spaces are missing around operators, else shouldn't be on a new line. Also go ahead and use interpolated string style in the log message, and pull out some helper vars for clarity.

Thank you, i'm dealing right away

srowen · 2017-04-19T10:31:21Z

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

@@ -497,11 +497,12 @@ private[spark] class BlockManagerInfo(

    updateLastSeenMs()

+    var originalMemSize: Long = 0


Why pull out a var here?

srowen · 2017-04-19T10:31:42Z

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

-        logInfo("Added %s in memory on %s (size: %s, free: %s)".format(
-          blockId, blockManagerId.hostPort, Utils.bytesToString(memSize),
-          Utils.bytesToString(_remainingMem)))
+        if((memSize-originalMemSize) >= 0){


if (memSize >= originalMemSize) is clearer and safer

Ok, i'm dealing right away

eatoncys · 2017-04-19T12:05:01Z

@srowen Thank you for suggestion, i have modified the style problems, any other suggestion?

srowen

Finally I think you might be able to further unify and standardize the log statements in the code below where it deals with just dropping the blocks. This should stay somewhat consistent.

srowen · 2017-04-19T12:07:22Z

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

@@ -520,8 +520,18 @@ private[spark] class BlockManagerInfo(
        blockStatus = BlockStatus(storageLevel, memSize = memSize, diskSize = 0)
        _blocks.put(blockId, blockStatus)
        _remainingMem -= memSize
-        logInfo("Added %s in memory on %s (size: %s, free: %s)".format(
-          blockId, blockManagerId.hostPort, Utils.bytesToString(memSize),
+        val originalMemSize = if (_blocks.containsKey(blockId)) {


I see, it's not a Scala Map or else this could be .getOrElse(..., 0). Now I understand why you pulled out the variable earlier. I actually dont' think this works now because it happens after the new status has been put in the Map. OK, you could go back to what you had.

srowen · 2017-04-19T12:09:20Z

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

+        }  else {
+          0
+        }
+        val (addedOrRemoved, size) = if (memSize >= originalMemSize) {


The difference doesn't need to be conditional. You can use math.abs

srowen · 2017-04-19T12:09:26Z

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

+        } else {
+          ("Removed", originalMemSize - memSize)
+        }
+        logInfo("%s %s in memory on %s (size: %s, free: %s)".format(


Still doesn't use interpolation

srowen · 2017-04-19T12:15:52Z

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

+          ("Removed", originalMemSize - memSize)
+        }
+        logInfo("%s %s in memory on %s (size: %s, free: %s)".format(
+          addedOrRemoved, blockId, blockManagerId.hostPort, Utils.bytesToString(size),
          Utils.bytesToString(_remainingMem)))
      }
      if (storageLevel.useDisk) {


Doesn't the disk case need a similar treatment?

Ok, I have modified the disk case, and add a "update" style log info, any other suggestions? thank you

eatoncys · 2017-04-19T12:51:44Z

@srowen I have modified the "Removed" to "Updated" if the block exists already, and keep the size to current block size. Add a "Updated" log info including current size and original size maybe better

srowen · 2017-04-20T07:26:43Z

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

-        logInfo("Added %s on disk on %s (size: %s)".format(
-          blockId, blockManagerId.hostPort, Utils.bytesToString(diskSize)))
+        if (blockExists) {
+          logInfo("Updated %s on disk on %s (current size: %s, original size %s)".format(


You are still not using string interpolation, please do

Sorry,i don't understand the meaning of "string interpolation" very clearly, is it like this: logInfo(s"Added $blockId on disk on ${blockManagerId.hostPort} (size: $diskSize)") ? but this is different from other log infos in this file

Yes, you've got it. It's worth changing all of the related log messages here, but not necessarily every one in the file. We are slowly updating to use string interpolation as this code gets updated.

Ok，i have modified it, thank you!

srowen · 2017-04-20T07:27:29Z

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

    if (_blocks.containsKey(blockId)) {
      // The block exists on the slave already.
      val blockStatus: BlockStatus = _blocks.get(blockId)
      val originalLevel: StorageLevel = blockStatus.storageLevel
-      val originalMemSize: Long = blockStatus.memSize
+      originalMemSize = blockStatus.memSize


BLocks of code even farther down can reuse these new values and need similar changes to the log comment

Ok,thanks for suggestion, I have modified it

srowen · 2017-04-21T08:04:53Z

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

@@ -497,11 +497,18 @@ private[spark] class BlockManagerInfo(

    updateLastSeenMs()

+    var blockExists = false


Although you could just assign val blockExists = _blocks.containsKey(blockId) once and reuse it instead of a var, it's minor. This looks good.

It's a good idea, i have modified it, thank you

SparkQA · 2017-04-21T08:07:32Z

Test build #3670 has finished for PR 17683 at commit 60ca9e0.

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

srowen · 2017-04-21T08:08:32Z

@eatoncys I think you have lines that are too long

eatoncys · 2017-04-21T09:50:29Z

@srowen Yes, i have two lines more than 100 characters, i have modified it, thank you

SparkQA · 2017-04-21T20:21:24Z

Test build #3672 has finished for PR 17683 at commit 664dfb8.

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

srowen · 2017-04-22T11:53:14Z

Merged to master/2.2

…the slave already ## What changes were proposed in this pull request? Modify the added memory size to memSize-originalMemSize if the block exists on the slave already since if the block exists, the added memory size should be memSize-originalMemSize; if originalMemSize is bigger than memSize ,then the log info should be Removed memory, removed size should be originalMemSize-memSize ## How was this patch tested? Multiple runs on existing unit tests (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Author: eatoncys <chen.yanshan@zte.com.cn> Closes #17683 from eatoncys/SPARK-20386. (cherry picked from commit 05a4514) Signed-off-by: Sean Owen <sowen@cloudera.com>

…the slave already ## What changes were proposed in this pull request? Modify the added memory size to memSize-originalMemSize if the block exists on the slave already since if the block exists, the added memory size should be memSize-originalMemSize; if originalMemSize is bigger than memSize ,then the log info should be Removed memory, removed size should be originalMemSize-memSize ## How was this patch tested? Multiple runs on existing unit tests (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Author: eatoncys <chen.yanshan@zte.com.cn> Closes apache#17683 from eatoncys/SPARK-20386.

PARK-20386

312b90b

srowen requested changes Apr 19, 2017

View reviewed changes

eatoncys added 2 commits April 19, 2017 19:04

modify the style problems and var problem

2d95652

modify the style problems and var problem

22ef469

srowen reviewed Apr 19, 2017

View reviewed changes

Change removed to updated

c14b1ad

eatoncys added 2 commits April 19, 2017 21:39

add 'updated' style log info

b769754

Delete blank line

81e8e84

srowen requested changes Apr 20, 2017

View reviewed changes

eatoncys added 3 commits April 20, 2017 16:23

Reuse the new values

567a9ae

Using string interpolation for log info

f7e40b8

Remove the blank line

60ca9e0

srowen approved these changes Apr 21, 2017

View reviewed changes

Modify the blockExists to val and resuse it below

664dfb8

asfgit closed this in 05a4514 Apr 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20386][Spark Core]modify the log info if the block exists on the slave already #17683

[SPARK-20386][Spark Core]modify the log info if the block exists on the slave already #17683

eatoncys commented Apr 19, 2017

srowen Apr 19, 2017

eatoncys Apr 19, 2017

srowen Apr 19, 2017

srowen Apr 19, 2017

eatoncys Apr 19, 2017

eatoncys commented Apr 19, 2017

srowen left a comment

srowen Apr 19, 2017

srowen Apr 19, 2017

srowen Apr 19, 2017

srowen Apr 19, 2017

eatoncys Apr 19, 2017

eatoncys commented Apr 19, 2017

srowen Apr 20, 2017

eatoncys Apr 20, 2017

srowen Apr 20, 2017

eatoncys Apr 20, 2017

srowen Apr 20, 2017

eatoncys Apr 20, 2017

srowen Apr 21, 2017

eatoncys Apr 21, 2017

SparkQA commented Apr 21, 2017

srowen commented Apr 21, 2017

eatoncys commented Apr 21, 2017 •

edited

Loading

SparkQA commented Apr 21, 2017

srowen commented Apr 22, 2017

		@@ -497,11 +497,12 @@ private[spark] class BlockManagerInfo(

		updateLastSeenMs()

		var originalMemSize: Long = 0

		@@ -497,11 +497,18 @@ private[spark] class BlockManagerInfo(

		updateLastSeenMs()

		var blockExists = false

[SPARK-20386][Spark Core]modify the log info if the block exists on the slave already #17683

[SPARK-20386][Spark Core]modify the log info if the block exists on the slave already #17683

Conversation

eatoncys commented Apr 19, 2017

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatoncys commented Apr 19, 2017

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatoncys commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 21, 2017

srowen commented Apr 21, 2017

eatoncys commented Apr 21, 2017 • edited Loading

SparkQA commented Apr 21, 2017

srowen commented Apr 22, 2017

eatoncys commented Apr 21, 2017 •

edited

Loading