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/10/26 23:05:35 UTC
[PR] [WIP][SPARK-45680][CONNECT] Release session [spark]
juliuszsompolski opened a new pull request, #43546:
URL: https://github.com/apache/spark/pull/43546
### What changes were proposed in this pull request?
WIP
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
### How was this patch tested?
### 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
Re: [PR] [SPARK-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1788501357
The test should pass now.
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1378275852
##########
python/pyspark/sql/tests/connect/test_connect_basic.py:
##########
@@ -3451,7 +3451,6 @@ def test_can_create_multiple_sessions_to_different_remotes(self):
# Gets currently active session.
same = PySparkSession.builder.remote("sc://other.remote.host:114/").getOrCreate()
self.assertEquals(other, same)
- same.stop()
Review Comment:
@juliuszsompolski I will take a look soon. Feel free to skip the tests for now, and we can just merge this first if this is an issue in tests only
--
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-45680][CONNECT] Release session [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1377960758
##########
python/pyspark/sql/tests/connect/test_connect_basic.py:
##########
@@ -3451,7 +3451,6 @@ def test_can_create_multiple_sessions_to_different_remotes(self):
# Gets currently active session.
same = PySparkSession.builder.remote("sc://other.remote.host:114/").getOrCreate()
self.assertEquals(other, same)
- same.stop()
Review Comment:
@HyukjinKwon could you suggest how to help with it?
The issue is that this test uses a bunch of sessions with dummy hosts, but these sessions used to never issue any RPCs, but now they do ReleaseSession in the stop, and then it notices that the host doesn't work and it fails...
I cannot simply remove the stop, because these tests actually test how stop()
In scala I resolved it with a bit of hacks in https://github.com/apache/spark/pull/43546/files#diff-a2b06ed831053e086e47d191c1e86d26da81c2d2a5cbaedd34a712297a2b418dR126
https://github.com/apache/spark/pull/43546/files#diff-a2b06ed831053e086e47d191c1e86d26da81c2d2a5cbaedd34a712297a2b418dR126 ...
--
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-45680][CONNECT] Release session [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1376366601
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -667,6 +667,7 @@ class SparkSession private[sql] (
* @since 3.4.0
*/
override def close(): Unit = {
+ client.releaseSession()
Review Comment:
Since client is an internal API that shouldn't be used to end user, I deliberately left it to the session layer to call release session. Here this allows me to test some internal behaviours with the client in the tests. In the future, I imagine this could be useful in some situations where we may need to exchange a client / get a new GRPC channel without closing the session.
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1788264993
I pushed some changes but I think you should take a look ...
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1786230748
I think we might need to retrigger ... https://github.com/juliuszsompolski/apache-spark/actions/runs/6695859696/job/18192423072
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1789860912
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
Re: [PR] [SPARK-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1788436733
hmmm .. `test_error_enrichment_jvm_stacktrace` seems a real test failure too. lemme take a look soon
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1378300936
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -667,6 +667,7 @@ class SparkSession private[sql] (
* @since 3.4.0
*/
override def close(): Unit = {
+ client.releaseSession()
Review Comment:
Or, should we explicitly connect when we create a session? Let me match the fix to this for now in any event.
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #43546: [SPARK-45680][CONNECT] Release session
URL: https://github.com/apache/spark/pull/43546
--
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-45680][CONNECT] Release session [spark]
Posted by "heyihong (via GitHub)" <gi...@apache.org>.
heyihong commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1376290191
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -667,6 +667,7 @@ class SparkSession private[sql] (
* @since 3.4.0
*/
override def close(): Unit = {
+ client.releaseSession()
Review Comment:
Should releaseSession be called inside `client.shutdown()`?
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -667,6 +667,7 @@ class SparkSession private[sql] (
* @since 3.4.0
*/
override def close(): Unit = {
+ client.releaseSession()
Review Comment:
Should `client.releaseSession()` be called inside `client.shutdown()`?
--
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-45680][CONNECT] Release session [spark]
Posted by "heyihong (via GitHub)" <gi...@apache.org>.
heyihong commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1376290689
##########
python/pyspark/sql/connect/session.py:
##########
@@ -654,6 +654,7 @@ def stop(self) -> None:
# multi-tenancy - the remote client side cannot just stop the server and stop
# other remote clients being used from other users.
with SparkSession._lock:
+ self.client.release_session()
Review Comment:
ditto
--
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-45680][CONNECT] Release session [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1786225354
@HyukjinKwon is this the same python test issue?
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43546:
URL: https://github.com/apache/spark/pull/43546#issuecomment-1788579388
rebased
--
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-45680][CONNECT] Release session [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43546:
URL: https://github.com/apache/spark/pull/43546#discussion_r1378300602
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -667,6 +667,7 @@ class SparkSession private[sql] (
* @since 3.4.0
*/
override def close(): Unit = {
+ client.releaseSession()
Review Comment:
I wonder if we should make it stoppable/closable ... even for dummy hosts, etc.
--
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