New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-17019][Core] Expose on-heap and off-heap memory usage in various places #14617
Conversation
Test build #63656 has finished for PR 14617 at commit
|
Test build #63657 has finished for PR 14617 at commit
|
Hi @jerryshao. I think this is a great idea and fills in an important gap in the app's UI. Going by the screenshot you posted, instead of putting both on and off heap memory in a single column, how about putting them in separate columns? For one thing, this would make them independently sortable. Also, how about splitting on- and off-heap memory in the summary table as well? Another missing piece of the puzzle is the total executor memory that executors report back to the master. For example, in the standalone master UI each app only reports allocated heap memory per executor. This is perhaps an even bigger omission as it misrepresents the actual memory footprint of the executors, and because of that the scheduler does not compute or report memory usage per worker correctly. This breaks memory resource allocation. Is this issue something your patch addresses? |
@mallman thanks a lot for your comments, I will change the UI to split into separate columns. Yes, as you mentioned current executor memory usage tracked in Standalone Master only shows on-heap memory, it is ideal to also count into off-heap memory, and this is already considered in yarn mode, so maybe we should also do it in Standalone mode. But I think it would be better to handle it in a separate PR. |
@mallman I changed the UI based on your comment, here is the new one (separate the on heap and off heap memory usage in two columns): |
Test build #64406 has finished for PR 14617 at commit
|
@jerryshao The UI changes look great. I have not had a chance to scrutinize the source changes. Hopefully we can get someone else to help review. |
Jenkins, retest this please. |
Test build #64450 has finished for PR 14617 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to the Blacklisting UI work I did recently as part of SPARK-16654 so I feel comfortable reviewing this code.
This largely UI change looks okay to me with a few nits (assuming, of course, it is updated to build correctly against master).
I think that the arrangement of the UI looks okay, with a separate column for on heap and off heap.
@@ -24,7 +24,10 @@ <h4 style="clear: left; display: inline-block;">Summary</h4> | |||
<th></th> | |||
<th>RDD Blocks</th> | |||
<th><span data-toggle="tooltip" | |||
title="Memory used / total available memory for storage of data like RDD partitions cached in memory. ">Storage Memory</span> | |||
title="Memory used / total available memory for on heap storage of data like RDD partitions cached in memory. "> On Heap Storage Memory</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: trailing space. Should me:
memory.">
title="Memory used / total available memory for on heap storage of data like RDD partitions cached in memory. "> On Heap Storage Memory</span> | ||
</th> | ||
<th><span data-toggle="tooltip" | ||
title="Memory used / total available memory for off heap storage of data like RDD partitions cached in memory. "> Off Heap Storage Memory</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit for trailing space.
var allOnHeapMemoryUsed = 0; | ||
var allOnHeapMaxMemory = 0; | ||
var allOffHeapMemoryUsed = 0; | ||
var allOffHeapMaxMemory = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct, and the order agrees with executorspage-template.html.
}, | ||
{ | ||
data: function (row, type) { | ||
return type === 'display' ? (formatBytes(row.offHeapMemoryUsed, type) + ' / ' + formatBytes(row.maxOffHeapMemory, type)) : row.maxOffHeapMemory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks correct, including the field names and using formatBytes.
} | ||
|
||
registerGauge(MetricRegistry.name("memory", "maxMem_MB"), | ||
_.getStorageStatus.map(_.maxMem).sum / 1024 / 1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related, but this change is the right time to get rid of "/1024/1024" magic number arithmetic. Ideally you could just do something like sum.toMegabytes, which pulls the arithmetic definition somewhere else. This is sprinkled through enough times that I'd like to see it encapsulated in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will change it.
private var _nonRddStorageInfo: (Long, Long) = (0L, 0L) | ||
|
||
// On-heap memory, off-heap memory and disk usage of non rdd storage | ||
private var _nonRddStorageInfo: (Long, Long, Long) = (0L, 0L, 0L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another preference, I'd rather see this as perhaps a case class so you could do something like:
_nonRddStorageInfo(onHeap: Long, offHeap: Long, disk: Long)
That way you don't need to do things like _1 and _2 below which, IMO, make the code less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about a case class to improve readability
onHeapMemUsed, | ||
offHeapMemUsed, | ||
maxOnHeapMem, | ||
maxOffHeapMem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks correct.
maxMem: Long, | ||
maxOnHeapMem: Option[Long] = None, | ||
maxOffHeapMem: Option[Long] = None) extends SparkListenerEvent { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that, with this, we get JSON serialization of SparkListenerBlockManagerAdded events for free? It's important that an external history server be able to see the new maxOnHeapMem and maxOffHeapMem values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so.
hi @jerryshao sorry this went unnoticed for so long, if you bring this up to date I'll keep an eye on it. Before this change, is off-heap storage completely ignored in the UI? Or does the UI just lump together off-heap and on-heap? I'm wondering if most users won't care about the distinction, and it is just extra noise in the UI. It certainly would be good to expose in the API for sure. cc @CodingCat @ajbozarth @tgravescs as you often have good ideas for the UI. |
ah...somehow I missed this email..... I like this idea, the comments here would be that how about exposing the all of total/on-heap/off-heap; and specifically, in UI, the default behavior would be displaying total, and user can check on-heap/off-heap details when the cursor hover around? just replace data-original-title in the following code snippet with the on-heap/off-heap memory details in some way (I think it is doable) <span data-toggle="tooltip" data-placement="top" title="" data-original-title="Memory used / total available memory for storage of data like RDD partitions cached in memory.">
Storage Memory</span> |
That's a good idea, thanks @CodingCat . |
Change the UI according to @CodingCat 's comment. |
Test build #74884 has finished for PR 14617 at commit
|
Looks like the failures are real (you probably just need to regenerate the expectations for the new blacklisting tests) |
@@ -172,6 +172,15 @@ function totalDurationColor(totalGCTime, totalDuration) { | |||
return (totalGCTime > GCTimePercent * totalDuration) ? "white" : "black"; | |||
} | |||
|
|||
function memoryUsageTooltip(onHeap, maxOnHeap, offHeap, maxOffHeap, onHeapSum, offHeapSum, type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onHeapSum
and offHeapSum
mean something completely different, right? aren't they more like totalMemoryUsed
and totalMemoryAvailable
?
" / " + formatBytes(maxOnHeap, type) + ") " + | ||
"Off Heap Memory (" + formatBytes(offHeap, type) + | ||
" / " + formatBytes(maxOffHeap, type) + ")'>" + | ||
formatBytes(onHeapSum, type) + " / " + formatBytes(offHeapSum, type) + "</span>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pretty hard to read (at least for me, though I don't really know js ...). Maybe refactor so you prepare two tmp variables, one with the tooltip text, and another one w/ the normal text, and then return the fully assembled string?
@@ -180,7 +180,8 @@ private[spark] class BlockManager( | |||
|
|||
val idFromMaster = master.registerBlockManager( | |||
id, | |||
maxMemory, | |||
maxOffHeapMemory, | |||
maxOffHeapMemory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of these should be on heap
|
||
/** Return the memory used by on-heap caching RDDs */ | ||
def onHeapCacheSize: Long = { | ||
_rddStorageInfo.filter(!_._2._3.useOffHeap).map(_._2._1).sum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine the filter & map with collect
. I think its a bit clearer and also avoids the extra intermediate structure.
_rddStorageInfo.collect {
case (_, (memoryUsed, _, storageStatus)) if !storageStatus.useOffHeap => memoryUsed
}.sum
while I kind of like the hover because it doesn't clutter the page, it does bring up a couple concerns:
The latter one is perhaps the one that is more concerning. Unless we can highlight it or somehow draw the users attention to it they will not know its there. But the first one is also pretty valid. If I have thousands of executors and looking for one that is out of off heap memory, its pretty difficult to hover over all of them. I don't see any changes to the Storage UI page, do we want to add something there as well? |
good points @tgravescs . What about making them additional metrics, turned on by a checkbox, like the extra task metrics? |
Thanks @tgravescs and @squito for your comments. Based on @tgravescs 's point, looks like making them as a table column is more valid. So I will revert back to use column and combine with checkbox. I personally have no strong preference, what's your opinion @CodingCat ? |
I agree with the checkpoint based solution , thanks for asking |
Checkbox sounds good to me. |
thanks @jerryshao . Two other points:
|
After reading through the previous comments I agree adding checkboxes to this page is a good idea, I would even suggest that we look at making checkboxes for a few of the current columns (default to show, to keep user compatibility)> I'm not sure which would be best but I know on many apps a few columns are never filled (Disk usage, and shuffle read/write first come to mind). |
Test build #75021 has finished for PR 14617 at commit
|
Test build #75022 has finished for PR 14617 at commit
|
@@ -276,7 +276,8 @@ class BlockManagerMasterEndpoint( | |||
|
|||
private def storageStatus: Array[StorageStatus] = { | |||
blockManagerInfo.map { case (blockManagerId, info) => | |||
new StorageStatus(blockManagerId, info.maxMem, info.blocks.asScala) | |||
new StorageStatus(blockManagerId, info.maxMem, Some(info.maxOnHeapMem), | |||
Some(info.maxOffHeapMem), info.blocks.asScala) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add one more field maxMem
. If using old event log to replay the storage info, only maxMem
field is valid. Other two fields (maxOnHeapMem
and maxOffHeapMem
) will be None.
val onHeapMemoryUsed: Option[Long], | ||
val offHeapMemoryUsed: Option[Long], | ||
val maxOnHeapMemory: Option[Long], | ||
val maxOffHeapMemory: Option[Long]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields now change to Option, it will not be printed out for old event log.
val onHeapMemoryUsed: Long, | ||
val offHeapMemoryUsed: Long, | ||
val onHeapMemoryRemaining: Long, | ||
val offHeapMemoryRemaining: Long) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields still use Long
instead of Option
, since only Live UI could access such REST API, and in the live UI everything is up to date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why the history server couldn't show this info. Though we would be fine now by having them be defined without Option, I worry that will be a problem if we ever change the history server to show this info, since it won't always have it available.
Test build #75444 has finished for PR 14617 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jerryshao thanks for the update, especially clarifying my concerns and that test. I think I have pretty small style / clarity comments this time, hopefully easy to take care of.
@@ -115,8 +115,9 @@ private[spark] object ExecutorsPage { | |||
val rddBlocks = status.numBlocks | |||
val memUsed = status.memUsed | |||
val maxMem = status.maxMem | |||
val onHeapMemUsed = status.onHeapMemUsed | |||
val offHeapMemUsed = status.offHeapMemUsed | |||
// Only maxOnHeapMem and maxOffHeapMem are defined these two fields are not None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment. Maybe you mean "status.onHeapMemUsed is only valid if maxOnHeapMem is defined"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry about the confuse. Let me clarify the comments.
@@ -81,6 +115,11 @@ private[spark] object ExecutorsPage { | |||
val rddBlocks = status.numBlocks | |||
val memUsed = status.memUsed | |||
val maxMem = status.maxMem | |||
// Only maxOnHeapMem and maxOffHeapMem are defined these two fields are not None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment. Maybe you mean "status.onHeapMemUsed is only valid if maxOnHeapMem is defined"?
val onHeapMemoryUsed: Long, | ||
val offHeapMemoryUsed: Long, | ||
val onHeapMemoryRemaining: Long, | ||
val offHeapMemoryRemaining: Long) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why the history server couldn't show this info. Though we would be fine now by having them be defined without Option, I worry that will be a problem if we ever change the history server to show this info, since it won't always have it available.
// existing issue, this happened when trying to replay old event log. | ||
val maxMemory: Long, | ||
val maxOnHeapMem: Option[Long], | ||
val maxOffHeapMem: Option[Long]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the comment you have here a little confusing. I'd get rid of it and put in something more like what you have where you create the StorageStatus
in BlockManagerMasterEndpoint
, something like
The onHeap and offHeap memory are always defined for new applications, but they can be missing if we are replaying old event logs.
}.sum | ||
|
||
/** Return the memory used by off-heap caching RDDs */ | ||
def offHeapCacheSize: Long = _rddStorageInfo.collect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should all methods that return something specific to onheap / offheap return an OPtion? It seems like if it was created from event logs, and the SparkListenerBlockManagerAdded even didn't have an onheap / offheap breakdown, we can't trust these numbers.
I think it doesn't matter now, in the code you have, since these methods won't be called unless you know you have an onheap / offheap breakdown available. But I think it would be a bit more clear if you made that explicit with the return types of these functions.
Change-Id: I09c2b26661f42334c095303ec8c76e821d4145d3
Thanks @squito , thanks so much for your review, just address the comments you mentioned. |
Test build #75539 has finished for PR 14617 at commit
|
lgtm, thanks for the updates any more comments @tgravescs @CodingCat @ajbozarth @jsoltren? |
This looks good to me. Thanks. |
merged to master. Thanks @jerryshao |
btw, anybody interested in looking at getting the memory to show up in the history server as well? this issue we were discussing earlier:
|
Thanks @squito . Regarding showing memory usage in history server. My major concern is that putting so many block update event into event log will significantly increase the file size and delay the replay, that's why in the current code we deliberately bypass the block update event. And IIUC in history server it is not necessary to show the change of used memory, only the final memory usage before application finished will be shown on the UI. So instead of recording and replaying all the block update events, just recording the final memory usage of each executor is enough. |
yeah, we definitely don't want to start logging more events. But it seems like this info is already available -- taskEnd.taskMetrics.updatedBlocks already has everything, doesn't it? |
I see. The current code leverages |
@squito , by revising this code, I found there're some places which are misleading and could be improved:
Either we should rename the field in REST API and others to specifically saying that is storage memory, or we should also count in execution memory usage.
What is your opinion about it, would be grateful to hear your comments. |
hi @jerryshao good points. First, we should probably move this discussion to jira so its more visible -- feel free to open two issues for these if you want, or first discuss on dev@. (Sorry its my fault for starting a discussion here in the PR comments after the original change was merged ...) On your first point, I like the idea of exposing more information on memory usage, but is there anything meaningful to report on execution memory in the rest api? It doesn't really seem like there is. Maybe we should rename the fields, but keep backwards compatibility in mind. For the second point about storage memory limits -- its a good question about what it should report with the Unified Memory Manager. I thought we'd just expose the portion of memory immune to eviction, Again, probably better to have the design discussion on jiras. |
## What changes were proposed in this pull request? This is a follow-up of #14617 to make the name of memory related fields more meaningful. Here for the backward compatibility, I didn't change `maxMemory` and `memoryUsed` fields. ## How was this patch tested? Existing UT and local verification. CC squito and tgravescs . Author: jerryshao <sshao@hortonworks.com> Closes #17700 from jerryshao/SPARK-20391. (cherry picked from commit 66dd5b8) Signed-off-by: Imran Rashid <irashid@cloudera.com>
## What changes were proposed in this pull request? This is a follow-up of apache#14617 to make the name of memory related fields more meaningful. Here for the backward compatibility, I didn't change `maxMemory` and `memoryUsed` fields. ## How was this patch tested? Existing UT and local verification. CC squito and tgravescs . Author: jerryshao <sshao@hortonworks.com> Closes apache#17700 from jerryshao/SPARK-20391.
## What changes were proposed in this pull request? #14617 added new columns to the executor table causing the visibility checks for the logs and threadDump columns to toggle the wrong columns since they used hard-coded column numbers. I've updated the checks to use column names instead of numbers so future updates don't accidentally break this again. Note: This will also need to be back ported into 2.2 since #14617 was merged there ## How was this patch tested? Manually tested Author: Alex Bozarth <ajbozart@us.ibm.com> Closes #17904 from ajbozarth/spark20630. (cherry picked from commit ca4625e) Signed-off-by: Sean Owen <sowen@cloudera.com>
## What changes were proposed in this pull request? #14617 added new columns to the executor table causing the visibility checks for the logs and threadDump columns to toggle the wrong columns since they used hard-coded column numbers. I've updated the checks to use column names instead of numbers so future updates don't accidentally break this again. Note: This will also need to be back ported into 2.2 since #14617 was merged there ## How was this patch tested? Manually tested Author: Alex Bozarth <ajbozart@us.ibm.com> Closes #17904 from ajbozarth/spark20630.
## What changes were proposed in this pull request? apache#14617 added new columns to the executor table causing the visibility checks for the logs and threadDump columns to toggle the wrong columns since they used hard-coded column numbers. I've updated the checks to use column names instead of numbers so future updates don't accidentally break this again. Note: This will also need to be back ported into 2.2 since apache#14617 was merged there ## How was this patch tested? Manually tested Author: Alex Bozarth <ajbozart@us.ibm.com> Closes apache#17904 from ajbozarth/spark20630.
What changes were proposed in this pull request?
With SPARK-13992, Spark supports persisting data into off-heap memory, but the usage of on-heap and off-heap memory is not exposed currently, it is not so convenient for user to monitor and profile, so here propose to expose off-heap memory as well as on-heap memory usage in various places:
Attach the UI changes:
Backward compatibility is also considered for event-log and REST API. Old event log can still be replayed with off-heap usage displayed as 0. For REST API, only adds the new fields, so JSON backward compatibility can still be kept.
How was this patch tested?
Unit test added and manual verification.