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/09/13 14:15:43 UTC

[GitHub] [spark] juliuszsompolski opened a new pull request, #42910: [SPARK-45133][FOLLOWUP] Add test that queries transition to FINISHED

juliuszsompolski opened a new pull request, #42910:
URL: https://github.com/apache/spark/pull/42910

   ### What changes were proposed in this pull request?
   
   Add test checking that queries (also special case: local relations) transition to FINISHED state, even if the client does not consume the results.
   
   ### Why are the changes needed?
   
   Add test for SPARK-45133.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   This adds tests.
   
   ### 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

[GitHub] [spark] juliuszsompolski commented on pull request #42910: [SPARK-45133][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1717722485

   cc @hvanhovell @HyukjinKwon @jdesjean 


-- 
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

[GitHub] [spark] juliuszsompolski commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1723405035

   Rebased on latest master, let CI rerun.


-- 
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

[GitHub] [spark] HyukjinKwon closed pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED
URL: https://github.com/apache/spark/pull/42910


-- 
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

[GitHub] [spark] juliuszsompolski commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1719194332

   @jdesjean ah, actually there's a deeper reason why this can't be tested in SparkConnectService: This tests that queries become FINISHED, even though client does not consume results.
   As I wrote above, SparkConnectService works with mocks. One thing that this mock doesn't have is flow control. The server will be pushing results to the output stream, regardless if the client consumes them or not.
   So to test a "non-result-consuming client, we need a suite implementing `SparkConnectServerTest` that will use this real RPC communication. For now this suite is the only one using this new infra. Maybe let me spin it off in another suite, since it's not related to Reattachable execute indeed?


-- 
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

[GitHub] [spark] HyukjinKwon commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1724617224

   Merged to master.


-- 
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

[GitHub] [spark] juliuszsompolski commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1719182176

   @jdesjean hm, I guess for this thing, it would be possible to test in SparkServiceSuite...
   The difference between this and SparkServiceSuite is that this uses a real client sending requests via RPCs to the server, while SparkServiceSuite is creating mock requests, and calling the server side RPC handlers directly with these request...
   
   > However, it seems difficult because executePlan is blocking.
   
   This is no longer true with reattachable execute (reattachable execute gets the RPC stream be handled by another thread), so theoretically this could be used. Even without reattachable execute, it could just launch it in a future.
   I'll take a look, you're right that it might fit better there.
   


-- 
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

[GitHub] [spark] juliuszsompolski commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1724046511

   https://github.com/juliuszsompolski/apache-spark/actions/runs/6223191775/job/16888681606 flake
   ```
   ======================================================================
   FAIL [44.498s]: test_listener_events (pyspark.sql.tests.connect.streaming.test_parity_listener.StreamingListenerParityTests)
   ----------------------------------------------------------------------
   Traceback (most recent call last):
     File "/__w/apache-spark/apache-spark/python/pyspark/sql/tests/connect/streaming/test_parity_listener.py", line 90, in test_listener_events
       self.check_terminated_event(terminated_event)
     File "/__w/apache-spark/apache-spark/python/pyspark/sql/tests/streaming/test_streaming_listener.py", line 61, in check_terminated_event
       self.assertEquals(event.exception, None)
   AssertionError: 'java.nio.channels.ClosedChannelException[9553 chars]1)\n' != None
   ```


-- 
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

[GitHub] [spark] jdesjean commented on pull request #42910: [SPARK-45133][CONNECT][TESTS][FOLLOWUP] Add test that queries transition to FINISHED

Posted by "jdesjean (via GitHub)" <gi...@apache.org>.
jdesjean commented on PR #42910:
URL: https://github.com/apache/spark/pull/42910#issuecomment-1718330877

   It be great if we could improve [SparkConnectServiceSuite](https://src.dev.databricks.com/databricks/runtime/-/blob/connector/connect/server/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectServiceSuite.scala?L247) instead of creating a new one. However, it seems difficult because executePlan is blocking. Logic lgtm


-- 
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