You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by liancheng <gi...@git.apache.org> on 2014/03/07 09:40:14 UTC
[GitHub] spark pull request: [SPARK-1194] Fix the same-RDD rule for cache r...
GitHub user liancheng opened a pull request:
https://github.com/apache/spark/pull/96
[SPARK-1194] Fix the same-RDD rule for cache replacement
SPARK-1194: https://spark-project.atlassian.net/browse/SPARK-1194
In the current implementation, when selecting candidate blocks to be swapped out, once we find a block from the same RDD that the block to be stored belongs to, cache eviction fails and aborts.
In this PR, we keep selecting blocks *not* from the RDD that the block to be stored belongs to until either enough free space can be ensured (cache eviction succeeds) or all such blocks are checked (cache eviction fails).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liancheng/spark fix-spark-1194
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/96.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 #96
----
commit 62c92ac7b8e616529bdaa52b73eb70e50bc01b47
Author: Cheng Lian <li...@gmail.com>
Date: 2014-03-07T08:32:47Z
Fixed SPARK-1194 https://spark-project.atlassian.net/browse/SPARK-1194
----
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37058765
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37058764
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10388411
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
--- End diff --
Agree, thanks.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37064311
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13052/
---
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-1194] Fix the same-RDD rule for cache r...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10386330
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
--- End diff --
Copy over this log message to L267/L277 - when there blocks in entries and eviction wont help store this block.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37046789
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37012311
One or more automated tests failed
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13042/
---
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10394826
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,18 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
--- End diff --
Thanks, removed :)
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37046790
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-1194] Fix the same-RDD rule for cache r...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37056228
@liancheng hey Cheng - good catch on this! Would you mind adding a unit test for this case? Checkout `BlockManagerSuite` there are several tests in there already. Ideally we'd have a test case that the old implementation failed but is handled correctly with your fix, just so it's crystal clear the case this is covering.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37064310
Merged build finished.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37091522
I decided not to merge this into 0.9... but maybe we could do that later if there is demand for it.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37091891
To be clear, I merged it into master so it will be in Spark 1.0.0. I just didn't back port it into the 0.9 branch so it won't be in Spark 0.9.1. Usually we backport things that are fairly high priority fixes, since any time there is some risk that a "fix" actually regresses behavior in some way. This one is a good fix but I'm not sure it's urgent enough - but we'll see if people running 0.9.X request this fix and if so we can backport it.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37052691
Merged build finished.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10387705
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
--- End diff --
@mridulm , I think LinkedHashMap actually keeps the order of insertion, but not using? (though I'm not clear how to track when the block is accessed by the tasks for now...)
---
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37091724
May I ask the reason? I understand that impact of this bug is limited.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10394468
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,18 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
--- End diff --
Hey @liancheng I think it's okay to remove this quote. If you look at the scaladoc it already explains the intended policy wrt to partitions in the same RDD - so I think that is sufficient. The scaladoc says "which leads to a wasteful cyclic replacement pattern for RDDs don't fit into memory that we want to avoid"
---
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10390021
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
+ rddToAdd match {
+ case Some(rddId) if rddId == getRddId(blockId) =>
--- End diff --
Made a mistake here, `rddId: Int == getRddId(blockId): Option[Int]` never holds...
---
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-1194] Fix the same-RDD rule for cache r...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10387464
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
+ rddToAdd match {
+ case Some(rddId) if rddId == getRddId(blockId) =>
+ // no-op
+ case _ =>
+ selectedBlocks += blockId
+ selectedMemory += pair.getValue.size
--- End diff --
Just a suggested alternative to LRU:
To minimize the number of affected RDDs, how about evicting the blocks from those RDDs occupying the most memory space first, because in usual, all the blocks in RDD are necessary for the computation, this approach may minimize the chance for recomputation
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-36980466
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10388071
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
--- End diff --
No, @mridulm `entries` is constructed with `accessOrder` set to be `true`, which enables LRU. Thanks for pointing this out @mridulm, wasn't aware of that before.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37090696
Merged build finished.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37090698
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13065/
---
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-1194] Fix the same-RDD rule for cache r...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10388297
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
--- End diff --
I see
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37089814
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-1194] Fix the same-RDD rule for cache r...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37093759
Thanks for this fix - excellent catch !
On Sat, Mar 8, 2014 at 1:53 PM, asfgit <no...@github.com> wrote:
> Closed #96 <https://github.com/apache/spark/pull/96> via 0b7b7fd<https://github.com/apache/spark/commit/0b7b7fd45cd9037d23cb090e62be3ff075214fe7>
> .
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/apache/spark/pull/96>
> .
>
---
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-1194] Fix the same-RDD rule for cache r...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/96#discussion_r10386811
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -236,13 +236,23 @@ private class MemoryStore(blockManager: BlockManager, maxMemory: Long)
while (maxMemory - (currentMemory - selectedMemory) < space && iterator.hasNext) {
val pair = iterator.next()
val blockId = pair.getKey
- if (rddToAdd.isDefined && rddToAdd == getRddId(blockId)) {
- logInfo("Will not store " + blockIdToAdd + " as it would require dropping another " +
- "block from the same RDD")
- return false
+ // Apply the same-RDD rule for cache replacement. Quoted from the
+ // original RDD paper:
+ //
+ // When a new RDD partition is computed but there is not enough
+ // space to store it, we evict a partition from the least recently
+ // accessed RDD, unless this is the same RDD as the one with the
+ // new partition. In that case, we keep the old partition in memory
+ // to prevent cycling partitions from the same RDD in and out.
+ //
+ // TODO implement LRU eviction
--- End diff --
entries is already a LinkedHashMap - so you iterate in LRU : you can remove the comment.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/96
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37089815
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37052692
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13049/
---
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37091921
Thanks for the explanation :)
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37012308
Merged build finished.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-36980467
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-1194] Fix the same-RDD rule for cache r...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37089290
@pwendell Regression test case added, also ensured that the old implementation fails on this test case.
---
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-1194] Fix the same-RDD rule for cache r...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/96#issuecomment-37091493
@liancheng thanks, looks great! I'll merge this.
---
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.
---