Skip to content

Conversation

@Earne
Copy link
Contributor

@Earne Earne commented Mar 22, 2016

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/SPARK-14055

How was this patch tested?

manual tests by running LiveJournalPageRank on a large dataset ( the dataset must larger enough to incure RDD partition eviction).

update writeLocksByTask when removeBlock(blockId)
@Earne Earne changed the title [SPARK-14055] writeLocksByTask need to be update [SPARK-14055] writeLocksByTask need to be update when removeBlock Mar 22, 2016
@JoshRosen
Copy link
Contributor

Jenkins, this is ok to test.

@Earne
Copy link
Contributor Author

Earne commented Mar 22, 2016

can we always calling blockManager.releaseLock(blockId) no matter newEffectiveStorageLevel is valid or not?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala#L360

@JoshRosen
Copy link
Contributor

Can we always calling blockManager.releaseLock(blockId) no matter newEffectiveStorageLevel is valid or not?

No, we have to hold the write lock while calling blockInfoManager.removeBlock(). We have to call BlockInfoManager.removeBlock() to ensure that we don't leak metadata for blocks which are fully removed.

@SparkQA
Copy link

SparkQA commented Mar 22, 2016

Test build #53739 has finished for PR 11875 at commit 679c731.

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

@holdenk
Copy link
Contributor

holdenk commented Mar 22, 2016

Would it maybe be better to have some text explaining the change than just a JIRA link in the PR description?

@JoshRosen
Copy link
Contributor

LGTM. Although a nicer description would be good, I think the patch's title is fairly self-explanatory. It also might be nice to have a regression test, but I can take care of that later during a planned future patch to refactor some locking internals in BlockInfoManager.

@JoshRosen
Copy link
Contributor

Merging this now. Thanks!

@asfgit asfgit closed this in 48ee16d Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants