cloud-fan commented on code in PR #58396:
URL: https://github.com/apache/spark/pull/58396#discussion_r4320692351
##########
python/pyspark/sql/tests/connect/client/test_client.py:
##########
@@ -623,11 +624,73 @@ def mock_close():
self.assertEqual(call_tracker["release_session"], 1)
self.assertEqual(call_tracker["close"], 1)
+ def test_on_exit_does_not_call_when_session_released(self):
+ client = SparkConnectClient("sc://foo/",
use_reattachable_execute=False)
+ client._release_session_on_exit = True
+ client._session_released = True
Review Comment:
**Nit (P3):** This fixture leaves `_server_session_id` at its default of
`None`. The new `not self._server_session_id` clause therefore makes
`_on_exit()` return by itself, and this test still passes if
`self._session_released` is dropped from the predicate. The unchanged
`test_on_exit_does_not_call_when_already_closed` has the same problem. It was
the only test isolating the `_closed` check from #56140, and it now passes
without that check too. No other test calls `_on_exit()` on a released or
closed client with a non-empty ID, and the assertion added to the deadline test
only covers the setter in `release_session()`.
Setting `client._server_session_id = "server-session-id"` in both negative
tests, as the other `_on_exit` tests now do, makes each one fail when its check
is removed.
See **Shared repair plan 1** in the review body.
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -2604,11 +2606,9 @@ def _delete_ml_cache(self, cache_ids: List[str],
evict_only: bool = False) -> Li
return []
def _on_exit(self) -> None:
- # If the client has already been explicitly closed, skip all cleanup
RPCs.
- # The server-side resources were released by close(); reissuing them
here
- # is wasted work and, if the server has since become unreachable, can
- # block process exit on the gRPC call.
- if self._closed:
+ # Skip cleanup RPCs if the session is already closed or released, or
was never
+ # established. The server-side session ID may be "" rather than None.
+ if self._closed or self._session_released or not
self._server_session_id:
Review Comment:
**Non-blocking (P2):** `_server_session_id` is only set in
`_verify_response_integrity`, but several supported paths create a live
server-side session without going through it:
- `SparkSession.addArtifacts()` / `copyFromLocalToFs()`: `ArtifactManager`
calls `AddArtifacts` on its own stub and reads only `response.artifacts`, while
the server handler calls `getOrCreateIsolatedSession`.
- `SparkSession.cloneSession()`: `clone()` returns a new client whose
`_server_session_id` is `None`, even though the server has already registered
the cloned session. `CloneSessionResponse.new_server_side_session_id` is
available but ignored.
- A non-plan RPC that fails (for example a failing `conf.get`) after the
server has already created the session.
For these clients the early return now skips the opt-in `release_session()`
and `close()` as well as the ML cache cleanup. For example, with
`SPARK_CONNECT_RELEASE_SESSION_ON_EXIT=true`, running `spark =
SparkSession.builder.remote(url).getOrCreate(); spark.addArtifacts("dep.py",
pyfile=True)` and then exiting used to release the session. Now the session and
the uploaded artifact stay on the server until
`spark.connect.session.manager.defaultSessionTimeout` (60m by default). That
conflicts with the description's "clients that used a session ... keep the
current behavior", and the comment's "was never established" does not hold for
these paths.
The default configuration is fine. These sessions have no ML cache entries,
so skipping `_cleanup_ml_cache()` for them loses nothing. Plan-bearing paths
are also fine, because every plan RPC first sends a verified Config request
that records the ID.
See **Shared repair plan 1** in the review body.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
