[SPARK-36242][CORE] Ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method #33460

LuciferYang · 2021-07-21T09:48:00Z

What changes were proposed in this pull request?

The main change of this pr is move writer.close() before success = true to ensure spill file closed before set success = true in ExternalSorter.spillMemoryIteratorToDisk method.

Why are the changes needed?

Avoid setting success = true first and then failure of close spill file

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins or GitHub Action
Add a new Test case to check The spill file should not exists if writer close fails

SparkQA · 2021-07-21T10:10:47Z

Test build #141402 has finished for PR 33460 at commit 1e970f3.

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

SparkQA · 2021-07-21T11:41:26Z

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

SparkQA · 2021-07-21T12:25:41Z

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

SparkQA · 2021-07-21T12:48:19Z

Kubernetes integration test unable to build dist.

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

SparkQA · 2021-07-21T14:41:58Z

Test build #141409 has finished for PR 33460 at commit f38d735.

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

mridulm

ExternalAppendOnlyMap does the right thing.
Looks like we missed it in ExternalSorter.

+CC @Ngone51

I would prefer if we had a test for this btw @LuciferYang.

LuciferYang · 2021-07-22T04:02:41Z

I would prefer if we had a test for this btw @LuciferYang.

How can I create a successful flush but fail to writer.close() case when calling this method?

writer is a local variable in ExternalSorter#spillMemoryIteratorToDisk method and spillMemoryIteratorToDisk is private method, so it's hard to mock writer`'s behavior.

Do you have any suggestions for this? @mridulm @HyukjinKwon @Ngone51

Or do we need more refactoring work to make this method easy to test?

mridulm · 2021-07-22T04:40:17Z

Mock blockManager.getDiskWriter ? See UnsafeShuffleWriterSuite for an example

LuciferYang · 2021-07-22T05:25:28Z

Mock blockManager.getDiskWriter ? See UnsafeShuffleWriterSuite for an example

Thx ~

LuciferYang · 2021-07-22T08:44:27Z

core/src/test/scala/org/apache/spark/util/collection/ExternalSorterSpillSuite.scala

+    ioe.getMessage.equals(errorMessage)
+    // The `TempShuffleBlock` create by diskBlockManager
+    // will remain before SPARK-36242
+    assert(!spillFilesCreated(0).exists())


@mridulm @Ngone51 @HyukjinKwon before this pr, assert(!spillFilesCreated(0).exists()) will failed, I tested it manually.

ExternalSorterSpillSuite.this.spillFilesCreated.apply(0).exists() was true ScalaTestFailureLocation: org.apache.spark.util.collection.ExternalSorterSpillSuite at (ExternalSorterSpillSuite.scala:137) org.scalatest.exceptions.TestFailedException: ExternalSorterSpillSuite.this.spillFilesCreated.apply(0).exists() was true at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472) at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471) at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231) at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295) at org.apache.spark.util.collection.ExternalSorterSpillSuite.$anonfun$new$1(ExternalSorterSpillSuite.scala:137)

Let me check this case with Scala 2.13

Let me check this case with Scala 2.13

Manual test passed

ExternalSorterSuite relies on 'LocalSparkContext', so I added a new test file

Thanks for the great work!

SparkQA · 2021-07-22T10:23:31Z

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

SparkQA · 2021-07-22T11:07:54Z

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

SparkQA · 2021-07-22T13:11:34Z

Test build #141483 has finished for PR 33460 at commit 65e6987.

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

Ngone51 · 2021-07-23T02:39:04Z

core/src/test/scala/org/apache/spark/util/collection/ExternalSorterSpillSuite.scala

+  private var taskContext: TaskContext = _
+
+  override protected def beforeEach(): Unit = {
+    tempDir = util.Utils.createTempDir(null, "test")


import Utils directly?

There is another Utils in org.apache.spark.util.collection package, so util.Utils is used here.

a57e76a change to import Utils directly and rename it to UUtils

Do you have any other suggestions for the naming of UUtils?

I see. Looks fine either way.

Ngone51 · 2021-07-23T02:39:40Z

core/src/test/scala/org/apache/spark/util/collection/ExternalSorterSpillSuite.scala

+        import org.apache.spark.storage.TempShuffleBlockId
+        import java.util.UUID


Move to the imports group?

SparkQA · 2021-07-23T03:59:34Z

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

SparkQA · 2021-07-23T04:33:09Z

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

SparkQA · 2021-07-23T05:56:36Z

Test build #141530 has finished for PR 33460 at commit a57e76a.

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

…ue` in `ExternalSorter.spillMemoryIteratorToDisk` method ### What changes were proposed in this pull request? The main change of this pr is move `writer.close()` before `success = true` to ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method. ### Why are the changes needed? Avoid setting `success = true` first and then failure of close spill file ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass the Jenkins or GitHub Action - Add a new Test case to check `The spill file should not exists if writer close fails` Closes #33460 from LuciferYang/external-sorter-spill-close. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: yi.wu <yi.wu@databricks.com> (cherry picked from commit f61d599) Signed-off-by: yi.wu <yi.wu@databricks.com>

Ngone51 · 2021-07-23T15:19:30Z

Thanks, merged to master/3.2.

dongjoon-hyun · 2021-07-23T19:12:15Z

+1, LGTM. Thank you, @LuciferYang and all!

dongjoon-hyun · 2021-07-23T19:15:06Z

BTW, this looks like a bug fix. Could you make a backport to branch-3.1/3.0 please?

LuciferYang · 2021-07-26T02:41:57Z

BTW, this looks like a bug fix. Could you make a backport to branch-3.1/3.0 please?

@dongjoon-hyun OK, I'll do it as soon as possible

…ue` in `ExternalSorter.spillMemoryIteratorToDisk` method ### What changes were proposed in this pull request? The main change of this pr is move `writer.close()` before `success = true` to ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method. ### Why are the changes needed? Avoid setting `success = true` first and then failure of close spill file ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass the Jenkins or GitHub Action - Add a new Test case to check `The spill file should not exists if writer close fails` Closes apache#33460 from LuciferYang/external-sorter-spill-close. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: yi.wu <yi.wu@databricks.com>

LuciferYang · 2021-07-26T03:29:44Z

@dongjoon-hyun
branch-3.1: #33513
branch-3.0: #33514

LuciferYang · 2021-07-26T03:37:20Z

thx ~ @Ngone51 @mridulm @dongjoon-hyun

move writer close to objectsWritten > 0 block

1e970f3

github-actions bot added the CORE label Jul 21, 2021

LuciferYang marked this pull request as draft July 21, 2021 09:49

LuciferYang marked this pull request as ready for review July 21, 2021 10:05

LuciferYang added 2 commits July 21, 2021 18:58

re-trigger jks

284df65

re-trigger jks

f38d735

mridulm approved these changes Jul 21, 2021

View reviewed changes

Ngone51 approved these changes Jul 22, 2021

View reviewed changes

LuciferYang added 3 commits July 22, 2021 16:37

Add a new test suite

701292e

Add a comments

4ce6b06

Add a comments

65e6987

LuciferYang commented Jul 22, 2021

View reviewed changes

Ngone51 reviewed Jul 23, 2021

View reviewed changes

LuciferYang added 2 commits July 23, 2021 10:58

move imports

93390ac

import util.Utils directly

a57e76a

Ngone51 closed this in f61d599 Jul 23, 2021

LuciferYang deleted the external-sorter-spill-close branch October 22, 2023 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36242][CORE] Ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method #33460

[SPARK-36242][CORE] Ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method #33460

LuciferYang commented Jul 21, 2021 •

edited

Loading

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

mridulm left a comment

LuciferYang commented Jul 22, 2021 •

edited

Loading

mridulm commented Jul 22, 2021 •

edited

Loading

LuciferYang commented Jul 22, 2021

LuciferYang Jul 22, 2021

LuciferYang Jul 22, 2021

LuciferYang Jul 22, 2021

Ngone51 Jul 23, 2021

SparkQA commented Jul 22, 2021

SparkQA commented Jul 22, 2021

SparkQA commented Jul 22, 2021

Ngone51 Jul 23, 2021

LuciferYang Jul 23, 2021 •

edited

Loading

Ngone51 Jul 23, 2021

LuciferYang Jul 23, 2021

Ngone51 Jul 23, 2021

LuciferYang Jul 23, 2021

LuciferYang Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

Ngone51 commented Jul 23, 2021

dongjoon-hyun commented Jul 23, 2021

dongjoon-hyun commented Jul 23, 2021

LuciferYang commented Jul 26, 2021

LuciferYang commented Jul 26, 2021

LuciferYang commented Jul 26, 2021

		import org.apache.spark.storage.TempShuffleBlockId
		import java.util.UUID

[SPARK-36242][CORE] Ensure spill file closed before set success = true in ExternalSorter.spillMemoryIteratorToDisk method #33460

[SPARK-36242][CORE] Ensure spill file closed before set success = true in ExternalSorter.spillMemoryIteratorToDisk method #33460

Conversation

LuciferYang commented Jul 21, 2021 • edited Loading

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?

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

mridulm left a comment

Choose a reason for hiding this comment

LuciferYang commented Jul 22, 2021 • edited Loading

mridulm commented Jul 22, 2021 • edited Loading

LuciferYang commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2021

SparkQA commented Jul 22, 2021

SparkQA commented Jul 22, 2021

Choose a reason for hiding this comment

LuciferYang Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

Ngone51 commented Jul 23, 2021

dongjoon-hyun commented Jul 23, 2021

dongjoon-hyun commented Jul 23, 2021

LuciferYang commented Jul 26, 2021

LuciferYang commented Jul 26, 2021

LuciferYang commented Jul 26, 2021

[SPARK-36242][CORE] Ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method #33460

[SPARK-36242][CORE] Ensure spill file closed before set `success = true` in `ExternalSorter.spillMemoryIteratorToDisk` method #33460

LuciferYang commented Jul 21, 2021 •

edited

Loading

LuciferYang commented Jul 22, 2021 •

edited

Loading

mridulm commented Jul 22, 2021 •

edited

Loading

LuciferYang Jul 23, 2021 •

edited

Loading