[SPARK-35543][CORE][FOLLOWUP] Fix memory leak in BlockManagerMasterEndpoint removeRdd #33020

attilapiros · 2021-06-22T09:41:05Z

What changes were proposed in this pull request?

Wrapping JHashMap[BlockId, BlockStatus] (used in blockStatusByShuffleService) into a new class BlockStatusPerBlockId which removes the reference to the map when all the persisted blocks are removed.

Why are the changes needed?

With #32790 a bug is introduced when all the persisted blocks are removed we remove the HashMap which already shared by the block manger infos but when new block is persisted this map is needed to be used again for storing the data (and this HashMap must be the same which shared by the block manger infos created for registered block managers running on the same host where the external shuffle service is).

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Extending BlockManagerInfoSuite with test which removes all the persisted blocks then adds another one.

attilapiros · 2021-06-22T09:42:05Z

cc @mridulm

SparkQA · 2021-06-22T09:46:04Z

Test build #140133 has finished for PR 33020 at commit 3537e9c.

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

SparkQA · 2021-06-22T10:34:41Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44661/

SparkQA · 2021-06-22T10:35:59Z

Test build #140135 has finished for PR 33020 at commit b78f726.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-22T10:43:01Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44661/

attilapiros · 2021-06-22T10:46:55Z

build error is unrelated:

[error] ## Exception when compiling 480 sources to
/home/jenkins/workspace/SparkPullRequestBuilder@2/sql/catalyst/target/scala-2.12/classes
[error] java.lang.StackOverflowError
[error] scala.reflect.api.Trees$Transformer.transform(Trees.scala:2563)
[error] scala.tools.nsc.transform.CleanUp$CleanUpTransformer.transform(CleanUp.scala:607)
[error] scala.tools.nsc.transform.CleanUp$CleanUpTransformer.transform(CleanUp.scala:37)
[error] scala.reflect.api.Trees$Transformer.$anonfun$transformStats$1(Trees.scala:2597)
[error] scala.reflect.api.Trees$Transformer.transformStats(Trees.scala:2595)
[error] scala.reflect.internal.Trees.itransform(Trees.scala:1430)
[error] scala.reflect.internal.Trees.itransform$(Trees.scala:1400)

Jenkins retest this please

SparkQA · 2021-06-22T11:20:46Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44662/

SparkQA · 2021-06-22T12:22:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44665/

SparkQA · 2021-06-22T12:32:47Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44665/

gengliangwang · 2021-06-22T13:07:50Z

@attilapiros Thanks for coming up with a better fix.
cc @Ngone51 as well

SparkQA · 2021-06-22T13:34:23Z

Test build #140138 has finished for PR 33020 at commit b78f726.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mridulm

Thanks for fixing this @attilapiros !

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

SparkQA · 2021-06-23T12:40:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44731/

SparkQA · 2021-06-23T12:49:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44731/

SparkQA · 2021-06-23T14:29:14Z

Test build #140203 has finished for PR 33020 at commit 9d8d357.

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

mridulm · 2021-06-24T05:03:48Z

Thanks for fixing this @attilapiros !
Thanks for the reviews @dongjoon-hyun, @Ngone51 :-)

…ching ### What changes were proposed in this pull request? Fixes a bug where if `spark.shuffle.service.fetch.rdd.enabled=true`, memory-only cached blocks will fail to unpersist. ### Why are the changes needed? In #33020, when all RDD blocks are removed from `externalShuffleServiceBlockStatus`, the underlying Map is nulled to reduce memory. When persisting blocks we check if it's using disk before adding it to `externalShuffleServiceBlockStatus`, but when removing them there is no check, so a memory-only cache block will keep `externalShuffleServiceBlockStatus` null, and when unpersisting it throw an NPE because it tries to remove from the null Map. This adds checks to the removal as well to only remove if the block is on disk, and therefore should have been added to `externalShuffleServiceBlockStatus` in the first place. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New and updated UT Closes #35959 from Kimahriman/fetch-rdd-memory-only-unpersist. Authored-by: Adam Binford <adamq43@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com>

…ching ### What changes were proposed in this pull request? Fixes a bug where if `spark.shuffle.service.fetch.rdd.enabled=true`, memory-only cached blocks will fail to unpersist. ### Why are the changes needed? In #33020, when all RDD blocks are removed from `externalShuffleServiceBlockStatus`, the underlying Map is nulled to reduce memory. When persisting blocks we check if it's using disk before adding it to `externalShuffleServiceBlockStatus`, but when removing them there is no check, so a memory-only cache block will keep `externalShuffleServiceBlockStatus` null, and when unpersisting it throw an NPE because it tries to remove from the null Map. This adds checks to the removal as well to only remove if the block is on disk, and therefore should have been added to `externalShuffleServiceBlockStatus` in the first place. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New and updated UT Closes #35959 from Kimahriman/fetch-rdd-memory-only-unpersist. Authored-by: Adam Binford <adamq43@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit e0939f0) Signed-off-by: Sean Owen <srowen@gmail.com>

…ching ### What changes were proposed in this pull request? Fixes a bug where if `spark.shuffle.service.fetch.rdd.enabled=true`, memory-only cached blocks will fail to unpersist. ### Why are the changes needed? In apache#33020, when all RDD blocks are removed from `externalShuffleServiceBlockStatus`, the underlying Map is nulled to reduce memory. When persisting blocks we check if it's using disk before adding it to `externalShuffleServiceBlockStatus`, but when removing them there is no check, so a memory-only cache block will keep `externalShuffleServiceBlockStatus` null, and when unpersisting it throw an NPE because it tries to remove from the null Map. This adds checks to the removal as well to only remove if the block is on disk, and therefore should have been added to `externalShuffleServiceBlockStatus` in the first place. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New and updated UT Closes apache#35959 from Kimahriman/fetch-rdd-memory-only-unpersist. Authored-by: Adam Binford <adamq43@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit e0939f0) Signed-off-by: Sean Owen <srowen@gmail.com>

Initial upload

3537e9c

github-actions bot added the CORE label Jun 22, 2021

fix checkstyle

b78f726

attilapiros changed the title ~~[WIP][SPARK-35543][CORE][FOLLOWUP] Fix memory leak in BlockManagerMasterEndpoint removeRdd~~ [SPARK-35543][CORE][FOLLOWUP] Fix memory leak in BlockManagerMasterEndpoint removeRdd Jun 22, 2021

mridulm approved these changes Jun 22, 2021

View reviewed changes

dongjoon-hyun approved these changes Jun 22, 2021

View reviewed changes

Ngone51 reviewed Jun 23, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala Show resolved Hide resolved

add comment

9d8d357

Ngone51 approved these changes Jun 23, 2021

View reviewed changes

asfgit closed this in 0bdece0 Jun 24, 2021

Kimahriman mentioned this pull request Mar 24, 2022

[SPARK-38640][CORE] Fix NPE with memory-only cache blocks and RDD fetching #35959

Closed

[SPARK-35543][CORE][FOLLOWUP] Fix memory leak in BlockManagerMasterEndpoint removeRdd #33020

[SPARK-35543][CORE][FOLLOWUP] Fix memory leak in BlockManagerMasterEndpoint removeRdd #33020

Uh oh!

Conversation

attilapiros commented Jun 22, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

attilapiros commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

attilapiros commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

gengliangwang commented Jun 22, 2021

Uh oh!

SparkQA commented Jun 22, 2021

Uh oh!

mridulm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Jun 23, 2021

Uh oh!

SparkQA commented Jun 23, 2021

Uh oh!

SparkQA commented Jun 23, 2021

Uh oh!

mridulm commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

attilapiros commented Jun 22, 2021 •

edited

Loading