You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by andrewor14 <gi...@git.apache.org> on 2014/06/28 08:15:41 UTC
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
GitHub user andrewor14 opened a pull request:
https://github.com/apache/spark/pull/1255
[SPARK-2307][Reprise] Correctly report RDD blocks on SparkUI
The existing code in `ExecutorPage.scala` requires a linear scan through all the blocks to filter out the uncached ones. Every refresh could be expensive if there are many blocks and many executors.
The proper semantics should be the following: `StorageStatusListener` should contain only block statuses that are cached. This means as soon as a block is unpersisted by any means, its status should be removed. This is reflected in the changes made in `StorageStatusListener.scala`.
The `StorageTab` must also be updated, because it currently detects dropped blocks only if their storage levels change to `StorageLevel.NONE`, which no longer happens, because now we simply remove their statuses.
If you have been following this chain of PRs like @pwendell, you will quickly notice that this reverts the changes in #1249, which reverts the changes in #1080. In other words, we are adding back the changes from #1080, and fixing SPARK-2307 on top of those changes. Please ask questions if you are confused.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andrewor14/spark storage-ui-fix-reprise
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1255.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1255
----
commit 3afde3fff4e07296b7fc9ddab50a76e3a321bf53
Author: Andrew Or <an...@gmail.com>
Date: 2014-06-28T06:01:25Z
Correctly report the number of blocks on SparkUI
This is actually quite tricky to get right. With this commit,
StorageStatusListener will only hold cached blocks (i.e. no blocks
with StorageLevel.NONE).
This means the StorageTab needs special handling, because it
currently relies on dropped blocks having StorageLevel.NONE, rather
than disappearing altogether in the storage status list.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325263
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
filteredStatus.foreach { storageStatus =>
updatedBlocks.foreach { case (blockId, updatedStatus) =>
- storageStatus.blocks(blockId) = updatedStatus
+ if (updatedStatus.storageLevel == StorageLevel.NONE) {
+ storageStatus.blocks.remove(blockId)
--- End diff --
Oh wait sorry, I guess this block is scoped only at the particular executor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47438524
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47420346
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47439477
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47439478
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16230/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325282
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
filteredStatus.foreach { storageStatus =>
updatedBlocks.foreach { case (blockId, updatedStatus) =>
- storageStatus.blocks(blockId) = updatedStatus
+ if (updatedStatus.storageLevel == StorageLevel.NONE) {
+ storageStatus.blocks.remove(blockId)
+ } else {
+ storageStatus.blocks(blockId) = updatedStatus
+ }
}
}
--- End diff --
Should you update the docs above `executorIdToStorageStatus` (above) to say that it only stores persisted blocks?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325924
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
--- End diff --
good point... I have no idea
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325927
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
filteredStatus.foreach { storageStatus =>
updatedBlocks.foreach { case (blockId, updatedStatus) =>
- storageStatus.blocks(blockId) = updatedStatus
+ if (updatedStatus.storageLevel == StorageLevel.NONE) {
+ storageStatus.blocks.remove(blockId)
+ } else {
+ storageStatus.blocks(blockId) = updatedStatus
+ }
}
}
--- End diff --
will do
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47419734
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47419736
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325276
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
--- End diff --
If you already have the `executorId` here, why doesn't this just directly index into the executorIdToStorageStatus instead of doing `find`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47420347
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16226/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47438520
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1255#issuecomment-47435516
@andrewor14 this looks good, but one thing, could we write some basic unit tests to cover the behavior of this listener (at least to test the specific case here). One of the major benefits of going through this event based model is that it should be pretty easy to write tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1255#discussion_r14325239
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala ---
@@ -37,7 +37,11 @@ class StorageStatusListener extends SparkListener {
val filteredStatus = storageStatusList.find(_.blockManagerId.executorId == execId)
filteredStatus.foreach { storageStatus =>
updatedBlocks.foreach { case (blockId, updatedStatus) =>
- storageStatus.blocks(blockId) = updatedStatus
+ if (updatedStatus.storageLevel == StorageLevel.NONE) {
+ storageStatus.blocks.remove(blockId)
--- End diff --
What if a block is replicated on more than one machine... will it have different ID's in either case? If not, what happens if the block is dropped multiple times on two different executors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2307][Reprise] Correctly report RDD blo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1255
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---