[SPARK-44872][CONNECT] Server testing infra and ReattachableExecuteSuite #42560
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
+46
to
+50
| // Other suites using mocks leave a mess in the global executionManager, | ||
| // shut it down so that it's cleared before starting server. | ||
| SparkConnectService.executionManager.shutdown() |
There was a problem hiding this comment.
The fact that we use singletons is ugly. I raised https://issues.apache.org/jira/browse/SPARK-44779 for it some time ago already.
HyukjinKwon
pushed a commit
that referenced
this pull request
Sep 7, 2023
…rror ### What changes were proposed in this pull request? Make INVALID_CURSOR.DISCONNECTED a retriable error. ### Why are the changes needed? This error can happen if two RPCs are racing to reattach to the query, and the client is still using the losing one. SPARK-44833 was a bug that exposed such a situation. That was fixed, but to be more robust, we can make this error retryable. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests will be added in #42560 ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42818 from juliuszsompolski/SPARK-44835. Authored-by: Juliusz Sompolski <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
HyukjinKwon
pushed a commit
that referenced
this pull request
Sep 7, 2023
…rror ### What changes were proposed in this pull request? Make INVALID_CURSOR.DISCONNECTED a retriable error. ### Why are the changes needed? This error can happen if two RPCs are racing to reattach to the query, and the client is still using the losing one. SPARK-44833 was a bug that exposed such a situation. That was fixed, but to be more robust, we can make this error retryable. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests will be added in #42560 ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42818 from juliuszsompolski/SPARK-44835. Authored-by: Juliusz Sompolski <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit f13743d) Signed-off-by: Hyukjin Kwon <[email protected]>
juliuszsompolski
force-pushed
the
sc-reattachable-tests
branch
from
a6e63a3 to
48aa78e
Compare
|
(deleting comment - was supposed to be on another PR) |
|
https://github.com/juliuszsompolski/apache-spark/actions/runs/6157633940/job/16708789265 flaky |
|
Alright merging. |
hvanhovell
pushed a commit
that referenced
this pull request
Sep 12, 2023
### What changes were proposed in this pull request? Add `SparkConnectServerTest` with infra to test real server with real client in the same process, but communicating over RPC. Add `ReattachableExecuteSuite` with some tests for reattachable execute. Two bugs were found by the tests: * Fix bug in `SparkConnectExecutionManager.createExecuteHolder` when attempting to resubmit an operation that was deemed abandoned. This bug is benign in reattachable execute, because reattachable execute would first send a ReattachExecute, which would be handled correctly in SparkConnectReattachExecuteHandler. For non-reattachable execute (disabled or old client), this is also a very unlikely scenario, because the retrying mechanism should be able to resubmit before the query is declared abandoned, and hence get an INVALID_HANDLE.OPERATION_ALREADY_EXISTS. This bug can manifest only if a non-reattachable execution is retried with so much delay that the operation was declared abandoned. * In `ExecuteGrpcResponseSender` there was an assertion that assumed that if `sendResponse` did not send, it was because deadline was reached. But it can also be because of interrupt. This would have resulted in interrupt returning an assertion error instead of CURSOR_DISCONNECTED in testing. Outside of testing assertions are not enabled, so this was not a problem outside of testing. ### Why are the changes needed? Testing of reattachable execute. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests added. Closes #42560 from juliuszsompolski/sc-reattachable-tests. Authored-by: Juliusz Sompolski <[email protected]> Signed-off-by: Herman van Hovell <[email protected]> (cherry picked from commit 4b96add) Signed-off-by: Herman van Hovell <[email protected]>
HyukjinKwon
pushed a commit
that referenced
this pull request
Sep 13, 2023
…st result task is finished ### What changes were proposed in this pull request? In the situation before, query will only be FINISHED when all results have been pushed into the output buffers (not necessarily received by client, but pushed out of the server). For LocalTableScanExec, post FINISHED before sending result batches, because nothing is executed, only cached local results are returned. For regular execution, post FINISHED after all task results have been returned from Spark, not after they have been processed and sent out. ### Why are the changes needed? Currently, even if a query finished running in Spark, it keeps being RUNNING until all results are sent. Then there is a very small difference between FINISHED and CLOSED. This change makes it behave more similar to e.g. Thriftserver. ### Does this PR introduce _any_ user-facing change? Yes. Queries will be posted as FINISHED when they finish executing, not when they finish sending results. ### How was this patch tested? Will add test in #42560 ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42889 from juliuszsompolski/SPARK-45133. Authored-by: Juliusz Sompolski <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
There was a problem hiding this comment.
Hi, @juliuszsompolski and @hvanhovell .
ReattachableExecuteSuite seems to be very unstable.
Since the commit to merge this PR, all CIs fails sequentially except only one doc PR.
- https://github.com/apache/spark/actions/runs/6167442896/job/16738435813
- https://github.com/apache/spark/actions/runs/6161837332/job/16734028280
- https://github.com/apache/spark/actions/runs/6161871902/job/16722114515
- https://github.com/apache/spark/actions/runs/6167186123/job/16737786210
- https://github.com/apache/spark/actions/runs/6166972473/job/16737277903
- https://github.com/apache/spark/actions/runs/6167442896/job/16738435813
|
Apologies. I ran the final version 10+ times on my machine without flakes, but it may be related to the CI runner having less resources. Looking. |
|
All the linked runs fail with the same: It doesn't reproduced for me, but I know what could be going on, so will tweak the test. |
|
Thank you for checking, @juliuszsompolski . |
|
@juliuszsompolski For the daily tests of Scala 2.13, in addition to https://github.com/apache/spark/actions/runs/6163764050/job/16728064919 |
This was referenced Sep 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add
SparkConnectServerTestwith infra to test real server with real client in the same process, but communicating over RPC.Add
ReattachableExecuteSuitewith some tests for reattachable execute.Two bugs were found by the tests:
SparkConnectExecutionManager.createExecuteHolderwhen attempting to resubmit an operation that was deemed abandoned. This bug is benign in reattachable execute, because reattachable execute would first send a ReattachExecute, which would be handled correctly in SparkConnectReattachExecuteHandler. For non-reattachable execute (disabled or old client), this is also a very unlikely scenario, because the retrying mechanism should be able to resubmit before the query is declared abandoned, and hence get an INVALID_HANDLE.OPERATION_ALREADY_EXISTS. This bug can manifest only if a non-reattachable execution is retried with so much delay that the operation was declared abandoned.ExecuteGrpcResponseSenderthere was an assertion that assumed that ifsendResponsedid not send, it was because deadline was reached. But it can also be because of interrupt. This would have resulted in interrupt returning an assertion error instead of CURSOR_DISCONNECTED in testing. Outside of testing assertions are not enabled, so this was not a problem outside of testing.Why are the changes needed?
Testing of reattachable execute.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Tests added.