[GitHub] [spark] nija-at commented on a diff in pull request #41013: [SPARK-43509][CONNECT] Support Creating multiple Spark Connect sessions

["nija-at (via GitHub)" <gi...@apache.org>]

nija-at commented on code in PR #41013:
URL: https://github.com/apache/spark/pull/41013#discussion_r1193998391


##########
python/pyspark/sql/tests/connect/test_connect_basic.py:
##########
@@ -3272,6 +3272,21 @@ def test_error_stack_trace(self):
         )
         spark.stop()
 
+    def test_can_create_multiple_sessions_to_different_remotes(self):

Review Comment:
   nit: test name and what it doesn't seem to align?



##########
python/pyspark/sql/tests/connect/test_connect_basic.py:
##########
@@ -3272,6 +3272,21 @@ def test_error_stack_trace(self):
         )
         spark.stop()
 
+    def test_can_create_multiple_sessions_to_different_remotes(self):

Review Comment:
   test to validate connecting to different remotes works is missing?



##########
python/pyspark/sql/connect/session.py:
##########
@@ -183,11 +179,17 @@ def getOrCreate(self) -> "SparkSession":
 
             if has_channel_builder:
                 assert self._channel_builder is not None
-                _active_spark_session = SparkSession(connection=self._channel_builder)
+                return SparkSession(connection=self._channel_builder)
             else:
                 spark_remote = to_str(self._options.get("spark.remote"))
                 assert spark_remote is not None
-                _active_spark_session = SparkSession(connection=spark_remote)
+                return SparkSession(connection=spark_remote)
+
+        def getOrCreate(self) -> "SparkSession":
+            global _active_spark_session
+            if _active_spark_session is not None:
+                return _active_spark_session
+            _active_spark_session = self.create()
             return _active_spark_session

Review Comment:
   Wouldn't it be better if `getOrCreate()` operated as would be expected from the API with such a name?
   
   That is, create a new session when a new URL is provided and return an existing session when a previous URL is provided.
   (Either cache with the URL as the key or spark_opts as the key.)
   
   `_active_spark_session` can either be first in or last in.



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