-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-34541][CORE] Fixed an issue where data could not be cleaned up when unregisterShuffle. #31664
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-34541][CORE] Fixed an issue where data could not be cleaned up when unregisterShuffle. #31664
Changes from 6 commits
bff7afd
da47187
da8facf
1e9c3b6
79f9a70
69467ee
7319255
e53ee15
a86b5ab
65be2c7
cd907bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,19 +17,27 @@ | |
|
|
||
| package org.apache.spark.shuffle.sort | ||
|
|
||
| import java.io.File | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
|
|
||
| import org.apache.commons.io.FileUtils | ||
| import org.apache.commons.io.filefilter.TrueFileFilter | ||
| import org.mockito.Mockito.{mock, when} | ||
| import org.mockito.invocation.InvocationOnMock | ||
| import org.mockito.stubbing.Answer | ||
| import org.scalatest.matchers.must.Matchers | ||
| import org.scalatest.matchers.should.Matchers._ | ||
|
|
||
| import org.apache.spark._ | ||
| import org.apache.spark.rdd.ShuffledRDD | ||
| import org.apache.spark.serializer.{JavaSerializer, KryoSerializer, Serializer} | ||
|
|
||
| /** | ||
| * Tests for the fallback logic in UnsafeShuffleManager. Actual tests of shuffling data are | ||
| * performed in other suites. | ||
| */ | ||
| class SortShuffleManagerSuite extends SparkFunSuite with Matchers { | ||
| class SortShuffleManagerSuite extends SparkFunSuite with Matchers with LocalSparkContext { | ||
|
|
||
| private def doReturn(value: Any) = org.mockito.Mockito.doReturn(value, Seq.empty: _*) | ||
|
|
||
|
|
@@ -131,4 +139,29 @@ class SortShuffleManagerSuite extends SparkFunSuite with Matchers { | |
| ))) | ||
| } | ||
|
|
||
| test("Data could not be cleaned up when unregisterShuffle") { | ||
|
||
| withTempDir { tmpDir => | ||
| val conf = new SparkConf(loadDefaults = false) | ||
| conf.set("spark.local.dir", tmpDir.getAbsolutePath) | ||
| sc = new SparkContext("local", "SPARK-34541", conf) | ||
| val rdd = sc.parallelize(1 to 10, 1).map(x => (x, x)) | ||
| // Create a shuffleRdd | ||
| val shuffledRdd = new ShuffledRDD[Int, Int, Int](rdd, new HashPartitioner(4)) | ||
| .setSerializer(new JavaSerializer(conf)) | ||
| def getAllFiles: Set[File] = | ||
| FileUtils.listFiles(tmpDir, TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE).asScala.toSet | ||
| val filesBeforeShuffle = getAllFiles | ||
| // Force the shuffle to be performed | ||
| shuffledRdd.count() | ||
| // Ensure that the shuffle actually created files that will need to be cleaned up | ||
| val filesCreatedByShuffle = getAllFiles -- filesBeforeShuffle | ||
| filesCreatedByShuffle.map(_.getName) should be | ||
| Set("shuffle_0_0_0.data", "shuffle_0_0_0.index") | ||
| // Check that the cleanup actually removes the files | ||
| sc.env.blockManager.master.removeShuffle(0, blocking = true) | ||
| for (file <- filesCreatedByShuffle) { | ||
| assert (!file.exists(), s"Shuffle file $file was not cleaned up") | ||
| } | ||
| } | ||
| } | ||
| } | ||
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: revert the unrelated change.
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.
line-133 is empty line, we add test in
SortShuffleManagerSuitebefore, after #31664 (comment), move the test toShuffleSuite, So i change the empty line together.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 know the reason, but it's still not a necessary change, especially when there's no other changes in the file.
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.
ok,i will revert it.