You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "juliuszsompolski (via GitHub)" <gi...@apache.org> on 2023/12/05 16:14:18 UTC
[PR] [SPARK-46186][FOLLOWUP] Remove flakiness of test in ReattachableExecuteSuite [spark]
juliuszsompolski opened a new pull request, #44189:
URL: https://github.com/apache/spark/pull/44189
### What changes were proposed in this pull request?
The test added in https://github.com/apache/spark/pull/44095 could be flaky because `MEDIUM_RESULTS_QUERY` could very quickly finish before interrupt was sent. Replace it with a query that sleeps 30 seconds, so that we are sure that interrupt runs before it finishes.
### Why are the changes needed?
Remove test flakiness.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Rerun ReattachableExecuteSuite 100+ times to check it isn't flaky.
### Was this patch authored or co-authored using generative AI tooling?
Github Copilot was assisting in some boilerplate auto-completion.
Generated-by: Github Copilot
--
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-46186][CONNECT][TESTS][FOLLOWUP] Remove flakiness of `ReattachableExecuteSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44189:
URL: https://github.com/apache/spark/pull/44189#issuecomment-1843516029
Thank you, @juliuszsompolski .
--
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-46186][CONNECT][TESTS][FOLLOWUP] Remove flakiness of `ReattachableExecuteSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #44189: [SPARK-46186][CONNECT][TESTS][FOLLOWUP] Remove flakiness of `ReattachableExecuteSuite`
URL: https://github.com/apache/spark/pull/44189
--
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-46186][FOLLOWUP] Remove flakiness of test in ReattachableExecuteSuite [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #44189:
URL: https://github.com/apache/spark/pull/44189#discussion_r1415893765
##########
connector/connect/server/src/test/scala/org/apache/spark/sql/connect/execution/ReattachableExecuteSuite.scala:
##########
@@ -309,12 +318,21 @@ class ReattachableExecuteSuite extends SparkConnectServerTest {
.setOperationId(operationId)
.build()
val iter = stub.executePlan(
- buildExecutePlanRequest(buildPlan(MEDIUM_RESULTS_QUERY), operationId = operationId))
+ buildExecutePlanRequest(buildPlan("select sleep(30000) as s"), operationId = operationId))
// wait for execute holder to exist, but the execute thread may not have started yet.
Eventually.eventually(timeout(eventuallyTimeout)) {
assert(SparkConnectService.executionManager.listExecuteHolders.length == 1)
}
stub.interrupt(interruptRequest)
+ // make sure the interrupt reaches the server
+ Eventually.eventually(timeout(eventuallyTimeout)) {
+ val execution = SparkConnectService.executionManager.listActiveExecutions match {
+ case Right(list) => list.find(_.operationId == operationId)
+ case Left(_) => None
+ }
+ assert(execution.isDefined && execution.get.status == ExecuteStatus.Canceled)
+ }
+ // make sure the client gets the OPERATION_CANCELED error
val e = intercept[StatusRuntimeException] {
while (iter.hasNext) iter.next()
Review Comment:
While raising PR I realize this is not needed, because the `sleep` query should ensure that we don't get results to iterate through for 30 seconds. By this time, interrupt should reach the server.
Will remove and test for flakiness again.
--
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-46186][FOLLOWUP] Remove flakiness of test in ReattachableExecuteSuite [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #44189:
URL: https://github.com/apache/spark/pull/44189#discussion_r1415893765
##########
connector/connect/server/src/test/scala/org/apache/spark/sql/connect/execution/ReattachableExecuteSuite.scala:
##########
@@ -309,12 +318,21 @@ class ReattachableExecuteSuite extends SparkConnectServerTest {
.setOperationId(operationId)
.build()
val iter = stub.executePlan(
- buildExecutePlanRequest(buildPlan(MEDIUM_RESULTS_QUERY), operationId = operationId))
+ buildExecutePlanRequest(buildPlan("select sleep(30000) as s"), operationId = operationId))
// wait for execute holder to exist, but the execute thread may not have started yet.
Eventually.eventually(timeout(eventuallyTimeout)) {
assert(SparkConnectService.executionManager.listExecuteHolders.length == 1)
}
stub.interrupt(interruptRequest)
+ // make sure the interrupt reaches the server
+ Eventually.eventually(timeout(eventuallyTimeout)) {
+ val execution = SparkConnectService.executionManager.listActiveExecutions match {
+ case Right(list) => list.find(_.operationId == operationId)
+ case Left(_) => None
+ }
+ assert(execution.isDefined && execution.get.status == ExecuteStatus.Canceled)
+ }
+ // make sure the client gets the OPERATION_CANCELED error
val e = intercept[StatusRuntimeException] {
while (iter.hasNext) iter.next()
Review Comment:
I realize this is not needed, because the `sleep` query should ensure that we don't get results to iterate through for 30 seconds. By this time, interrupt should reach the server.
--
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-46186][CONNECT][TESTS][FOLLOWUP] Remove flakiness of `ReattachableExecuteSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44189:
URL: https://github.com/apache/spark/pull/44189#issuecomment-1841217881
Thank you, @juliuszsompolski .
--
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