[SPARK-29161][CORE][SQL][STREAMING] Unify default wait time for waitUntilEmpty #25837
Conversation
|
Test build #110942 has finished for PR 25837 at commit
|
| * Exposed for testing. | ||
| */ | ||
| @throws(classOf[TimeoutException]) | ||
| def waitUntilEmpty(): Unit = { |
There was a problem hiding this comment.
can we make it private[spark]?
HeartSaVioR
changed the title
|
Looks good if tests pass. |
|
Test build #110945 has finished for PR 25837 at commit
|
|
"metadata not propagated" not relevant to this patch. |
|
retest this, please |
|
Test build #110950 has finished for PR 25837 at commit
|
|
Test build #110956 has finished for PR 25837 at commit
|
| @@ -180,7 +180,7 @@ class CoarseGrainedSchedulerBackendSuite extends SparkFunSuite with LocalSparkCo | |||
| backend.driverEndpoint.askSync[Boolean]( | |||
| RegisterExecutor("3", mockEndpointRef, mockAddress.host, 1, logUrls, attributes, Map.empty)) | |||
|
|
|||
| sc.listenerBus.waitUntilEmpty(executorUpTimeout.toMillis) | |||
| sc.listenerBus.waitUntilEmpty() | |||
There was a problem hiding this comment.
For my understanding, is this reducing the original timeout because it was 1 minutes before (private val executorUpTimeout = 1.minute)? It seems that this file's two instances are the only place to reduce the timeout unlike the other places.
There was a problem hiding this comment.
Ah right. My bad that's 60000 ms. I'll just roll them back.
|
Since this passed Jenkins already and the last commit is recovered to the original code, we don't need to wait the new Jenkins. The current one also will fail at in an hour (at midnight PST). I'll merge this to master. Thank you, @HeartSaVioR , @HyukjinKwon , @gaborgsomogyi , @srowen . |
|
Test build #111045 has finished for PR 25837 at commit
|
|
Thanks all for reviewing and merging! |
HeartSaVioR
deleted the
MINOR-unify-default-wait-time-for-wait-until-empty
branch
What changes were proposed in this pull request?
This is a follow-up of the review comment.
This patch unifies the default wait time to be 10 seconds as it would fit most of UTs (as they have smaller timeouts) and doesn't bring additional latency since it will return if the condition is met.
This patch doesn't touch the one which waits 100000 milliseconds (100 seconds), to not break anything unintentionally, though I'd rather questionable that we really need to wait for 100 seconds.
Why are the changes needed?
It simplifies the test code and get rid of various heuristic values on timeout.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI build will test the patch, as it would be the best environment to test the patch (builds are running there).