You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "attilapiros (via GitHub)" <gi...@apache.org> on 2024/10/28 18:54:19 UTC
[PR] [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite [spark]
attilapiros opened a new pull request, #48683:
URL: https://github.com/apache/spark/pull/48683
### What changes were proposed in this pull request?
Fixing race condition in the test "SPARK-46957: Migrated shuffle files should be able to cleanup from executor" of `BlockManagerDecommissionIntegrationSuite`.
There are at least two race conditions in this test:
1) The `SparkListener` is running in a different thread from the main thread of the unit test so `shuffleBlockUpdates` must be accessed from synchronised block. For this `ConcurrentLinkedQueue` is used similarly to `taskEndEvents` in the `runDecomTest()` method. There was not reported error for this yet.
2) The `SparkListener` is informed earlier about a removed executor than the block manager `stop()` is called/finished. So latter when the test recursively iterating over the files to find the shuffle files the block manager's `stop()` might be running and deleting the underlying files recursively via the disk block manager. This leads to `java.nio.file.NoSuchFileException`, like:
```
- SPARK-46957: Migrated shuffle files should be able to cleanup from executor *** FAILED ***
18848 java.io.UncheckedIOException: java.nio.file.NoSuchFileException: /home/runner/work/spark/spark/core/target/tmp/spark-87f59bc6-b996-42cd-9775-2f704b67f773/executor-e0a030d4-434b-46b3-bdbf-81d4908bb0f5/blockmgr-d88ed5dd-c1c8-4713-9433-10694e736a8e/3a
18849 at java.base/java.nio.file.FileTreeIterator.fetchNextIfNeeded(FileTreeIterator.java:87)
18850 at java.base/java.nio.file.FileTreeIterator.hasNext(FileTreeIterator.java:103)
18851 at java.base/java.util.Iterator.forEachRemaining(Iterator.java:132)
18852 at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1939)
18853 at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
18854 at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
18855 at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
18856 at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
18857 at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
18858 at org.apache.commons.io.FileUtils.toList(FileUtils.java:3025)
18859 ...
18860 Cause: java.nio.file.NoSuchFileException: /home/runner/work/spark/spark/core/target/tmp/spark-87f59bc6-b996-42cd-9775-2f704b67f773/executor-e0a030d4-434b-46b3-bdbf-81d4908bb0f5/blockmgr-d88ed5dd-c1c8-4713-9433-10694e736a8e/3a
18861 at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
18862 at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
18863 at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
18864 at java.base/sun.nio.fs.UnixFileAttributeViews$Basic.readAttributes(UnixFileAttributeViews.java:55)
18865 at java.base/sun.nio.fs.UnixFileSystemProvider.readAttributes(UnixFileSystemProvider.java:171)
18866 at java.base/sun.nio.fs.LinuxFileSystemProvider.readAttributes(LinuxFileSystemProvider.java:99)
18867 at java.base/java.nio.file.Files.readAttributes(Files.java:1853)
18868 at java.base/java.nio.file.FileTreeWalker.getAttributes(FileTreeWalker.java:226)
18869 at java.base/java.nio.file.FileTreeWalker.visit(FileTreeWalker.java:277)
18870 at java.base/java.nio.file.FileTreeWalker.next(FileTreeWalker.java:374)
18871 ...
```
To address this issue the local directory used by the decommissioned executor was find out and the wait is changed from getting the `ExecutorRemoved` event to wait for the delete of this directory to be finished. This way the directory tree walked after the delete.
Disclaimer:
The jira also mentions failures like:
```
[info] - SPARK-46957: Migrated shuffle files should be able to cleanup from executor *** FAILED *** (35 seconds, 200 milliseconds)
15718[info] 0 was not greater than or equal to 4 (BlockManagerDecommissionIntegrationSuite.scala:423)
15719[info] org.scalatest.exceptions.TestFailedException:
15720[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
15721[info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
15722[info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
15723[info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
15724[info] at org.apache.spark.storage.BlockManagerDecommissionIntegrationSuite.$anonfun$new$10(BlockManagerDecommissionIntegrationSuite.scala:423)
```
In my reproduction only the failure with `java.nio.file.NoSuchFileException` was coming so it might be there are still something to fix even after this changes.
### Why are the changes needed?
As Maven daily test with Java 21 was failing from time to time (flaky because of the race condition).
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
It was tested by running the existing unit test repeatedly.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #48683:
URL: https://github.com/apache/spark/pull/48683#discussion_r1819597117
##########
core/src/test/scala/org/apache/spark/storage/BlockManagerDecommissionIntegrationSuite.scala:
##########
@@ -377,20 +379,22 @@ class BlockManagerDecommissionIntegrationSuite extends SparkFunSuite with LocalS
.set(config.STORAGE_DECOMMISSION_SHUFFLE_BLOCKS_ENABLED, true)
sc = new SparkContext(conf)
TestUtils.waitUntilExecutorsUp(sc, 2, 60000)
- val shuffleBlockUpdates = new ArrayBuffer[BlockId]()
- var isDecommissionedExecutorRemoved = false
+ val shuffleBlockUpdates = new ConcurrentLinkedQueue[BlockId]()
val execToDecommission = sc.getExecutorIds().head
+ val decommissionedExecutorLocalDir = sc.parallelize(1 to 10, 100).flatMap { _ =>
Review Comment:
Just a question. Why `1 to 10` and `100`? Could you add a comment about this logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #48683: [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite
URL: https://github.com/apache/spark/pull/48683
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite [spark]
Posted by "attilapiros (via GitHub)" <gi...@apache.org>.
attilapiros commented on code in PR #48683:
URL: https://github.com/apache/spark/pull/48683#discussion_r1819712363
##########
core/src/test/scala/org/apache/spark/storage/BlockManagerDecommissionIntegrationSuite.scala:
##########
@@ -377,20 +379,22 @@ class BlockManagerDecommissionIntegrationSuite extends SparkFunSuite with LocalS
.set(config.STORAGE_DECOMMISSION_SHUFFLE_BLOCKS_ENABLED, true)
sc = new SparkContext(conf)
TestUtils.waitUntilExecutorsUp(sc, 2, 60000)
- val shuffleBlockUpdates = new ArrayBuffer[BlockId]()
- var isDecommissionedExecutorRemoved = false
+ val shuffleBlockUpdates = new ConcurrentLinkedQueue[BlockId]()
val execToDecommission = sc.getExecutorIds().head
+ val decommissionedExecutorLocalDir = sc.parallelize(1 to 10, 100).flatMap { _ =>
Review Comment:
It does not really matter but it really looks odd.
Let me change that to `(1 to 100, 10)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49297][CORE][TESTS] Fix race condition in BlockManagerDecommissionIntegrationSuite [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #48683:
URL: https://github.com/apache/spark/pull/48683#issuecomment-2443153867
After a minor import statement merging, I verified this manually.
```scala
- import java.nio.file.Files
- import java.nio.file.Paths
+ import java.nio.file.{Files, Paths}
```
```
[info] BlockManagerDecommissionIntegrationSuite:
[info] - SPARK-32850: BlockManager decommission should respect the configuration (enabled=false) (8 seconds, 477 milliseconds)
[info] - SPARK-32850: BlockManager decommission should respect the configuration (enabled=true) (7 seconds, 182 milliseconds)
[info] - verify that an already running task which is going to cache data succeeds on a decommissioned executor after task start (11 seconds, 871 milliseconds)
[info] - verify that an already running task which is going to cache data succeeds on a decommissioned executor after one task ends but before job ends (2 seconds, 884 milliseconds)
[info] - verify that shuffle blocks are migrated (3 seconds, 504 milliseconds)
[info] - verify that both migrations can work at the same time (3 seconds, 376 milliseconds)
[info] - SPARK-36782 not deadlock if MapOutput uses broadcast (3 seconds, 362 milliseconds)
[info] - SPARK-46957: Migrated shuffle files should be able to cleanup from executor (32 seconds, 227 milliseconds)
[info] Run completed in 1 minute, 13 seconds.
[info] Total number of tests run: 8
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 8, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 82 s (01:22), completed Oct 28, 2024, 9:01:32 PM
```
Merged to master for Apache Spark 4.0.0 on February 2025.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org