[SPARK-53622][CORE][TEST] Improve UninterruptibleThreadSuite
#52373
Conversation
|
@cloud-fan @Ngone51 Please take a look |
dongjoon-hyun
changed the title
UninterruptibleThreadSuite
| } | ||
|
|
||
| test("stress test") { | ||
| for (i <- 0 until 20) { |
There was a problem hiding this comment.
Is this the major change tries to improve the test coverage? Since it's executed in sequence, the stress of interruptions to a single UninterruptibleThread doesn't seem to be inscreased to me.
There was a problem hiding this comment.
This change does not target to increase concurrency of the stress test. It targets to reproduce SPARK-53394
| } | ||
|
|
||
| /* Await latch and return true if it's interrupted */ | ||
| private def await(latch: CountDownLatch, timeout: Long = 10, |
There was a problem hiding this comment.
What's the major difference after using this await()?
There was a problem hiding this comment.
We can equally use sleep() and await() to test for InterruptedException as they both throw an exception in case thread is interrupted. The difference is that sleep() does not return while await() will exit once the main thread calls interrupt() and count down the latch.
| timeUnit: TimeUnit = TimeUnit.SECONDS): Boolean = { | ||
| try { | ||
| if (!latch.await(timeout, timeUnit)) { | ||
| log.error("timeout while waiting for the latch") |
There was a problem hiding this comment.
It's test, and this log seems not useful as we fail right after it with the same error message.
There was a problem hiding this comment.
When await is used not in the main test thread, fail() does not cause thread to fail, it terminates the thread with TestFailedException (that is logged to stderr). And when this happens test fails as other conditions are not met. The log.error() logs to unit-tests.log, so there is no duplication of the error message. Note that in this case, the code is similar to:
log.error("message")
throw new Exception("message")
What changes were proposed in this pull request?
Implement several code improvement in
UninterruptibleThreadSuite:InterruptedExceptionasserterror message ("false did not equal true" => "hasInterruptedException was false")awaitinstead ofsleepWhy are the changes needed?
improve test coverage of
UninterruptibleThreadand help with test failure troubleshootingDoes this PR introduce any user-facing change?
No
How was this patch tested?
Run modified test
Was this patch authored or co-authored using generative AI tooling?
No