You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by "ericm-db (via GitHub)" <gi...@apache.org> on 2026/07/31 18:22:13 UTC

[PR] [SPARK-58021][CONNECT] Add local server pool lifecycle management [spark]

ericm-db opened a new pull request, #57686:
URL: https://github.com/apache/spark/pull/57686

### What changes were proposed in this pull request?

This is layer 3 of the local Connect pool stack: #57684 -> #57685 -> this PR. Until the lower layers merge, GitHub shows their cumulative diff; the new review unit here is commit `76def5665b6`.

This layer adds process lifecycle management on top of the storage model:

- janitor rules for dead or timed-out launches;
- retirement of dead, unreachable, idle, and orphaned claimed servers;
- graceful termination followed by SIGKILL escalation;
- cleanup of stale member directories and half-started daemons;
- idempotent release of a claimed member; and
- a forceful pool purge escape hatch.

It does not add background acquisition or SparkSession dispatch.

### Why are the changes needed?

Pool members outlive individual Python call frames and can be orphaned by crashes or SIGKILL. Their cleanup rules need to be explicit, idempotent, and testable before attendants and acquisition loops begin invoking them concurrently.

### Does this PR introduce _any_ user-facing change?

No. The pool is not wired into SparkSession in this layer.

### How was this patch tested?

Expanded `LocalConnectServerPoolUnitTests` with fake-process coverage for dead and live pending launches, unreachable and idle members, orphaned claims, shutdown escalation, release, and purge.

```bash
python -m unittest -v pyspark.sql.tests.connect.test_connect_local_server_pool.LocalConnectServerPoolUnitTests
```

All 14 tests passed. Ruff check, Ruff format check, and `git diff --check` also passed.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5)

--
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor closed pull request #57686: [SPARK-58021][CONNECT] Add local pool server retirement
URL: https://github.com/apache/spark/pull/57686


-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "ericm-db (via GitHub)" <gi...@apache.org>.

ericm-db commented on PR #57686:
URL: https://github.com/apache/spark/pull/57686#issuecomment-5416812998

   Thanks for the thorough review. Addressed in `0fb7fff3421`:
   
   - Atomic writes: fixed descriptor ownership and temporary-file cleanup.
   - Recovery: preserved PID/generation pairing and kept identity-less PIDs non-actionable.
   - Lifecycle: retry SIGTERM before escalation and tolerate invalid state entries.
   - Cleanup: fixed imports, removed dead code, clarified contracts, and expanded coverage.
   
   The focused tests, Python compilation, and Ruff checks pass.


-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on PR #57686:
URL: https://github.com/apache/spark/pull/57686#issuecomment-5446240773

   **Merge Summary:**
   - merged into master https://github.com/apache/spark/commit/a1857f57522e2cf5e870cfef3ecb0b51ecbf5325
   - merged into branch-4.x https://github.com/apache/spark/commit/59e7076efead490b0dbc3ec28b4baac8f7708a8e
   
   *Posted by `merge_spark_pr.py`*


-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on code in PR #57686:
URL: https://github.com/apache/spark/pull/57686#discussion_r3857485444


##########
python/pyspark/sql/connect/local_server_pool.py:
##########
@@ -101,19 +113,84 @@ def resolved(command: str) -> str:
     return hashlib.sha256(json.dumps(identity).encode("utf-8")).hexdigest()
 
 
-# The end of year 9999 UTC, as a Unix timestamp. ``created`` is a wall-clock ``time.time()``
-# reading, so no real clock reaches this for millennia; rejecting values beyond it keeps a
-# corrupt far-future timestamp from looking perpetually fresh to age-based reaping in the
-# layers above, which measure a member's age as ``time.time() - created``.
-_MAX_CREATED = 253402300799
+class _PoolStateRecord:

Review Comment:
   Please remove the leading underscore from the class name, and add a class comment describing what the class represents as an object and who uses it? Same for RetiredState and other classes in this file.



-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "ericm-db (via GitHub)" <gi...@apache.org>.

ericm-db commented on code in PR #57686:
URL: https://github.com/apache/spark/pull/57686#discussion_r3855996965


##########
python/pyspark/sql/tests/connect/test_connect_local_server_pool.py:
##########
@@ -65,15 +67,47 @@ def _non_listening_socket():
 def _spawn_live_process() -> "subprocess.Popen":
     """A child blocked on its parent pipe, standing in for a live pool server."""
     return subprocess.Popen(
-        [sys.executable, "-c", "import sys; sys.stdin.buffer.read()"],
+        [sys.executable, "-c", "import sys; sys.stdin.buffer.read()", _SERVER_CLASS],
         stdin=subprocess.PIPE,
         stdout=subprocess.DEVNULL,
         stderr=subprocess.DEVNULL,
     )
 
 
+def _spawn_stubborn_sleeper() -> "subprocess.Popen":
+    """A sleeper that ignores SIGTERM, standing in for a server hanging in shutdown. It
+    prints one line once its handler is installed so tests do not signal it too early."""
+    proc = subprocess.Popen(
+        [
+            sys.executable,
+            "-c",
+            "import signal, time\n"
+            "signal.signal(signal.SIGTERM, signal.SIG_IGN)\n"
+            "print('ready', flush=True)\n"
+            "time.sleep(300)",

Review Comment:
   Yup good idea, done



-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on PR #57686:
URL: https://github.com/apache/spark/pull/57686#issuecomment-5446224839

   LGTM, merging to master + 4.x


-- 
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-58021][CONNECT] Add local pool server retirement [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on code in PR #57686:
URL: https://github.com/apache/spark/pull/57686#discussion_r3855564534


##########
python/pyspark/sql/tests/connect/test_connect_local_server_pool.py:
##########
@@ -65,15 +67,47 @@ def _non_listening_socket():
 def _spawn_live_process() -> "subprocess.Popen":
     """A child blocked on its parent pipe, standing in for a live pool server."""
     return subprocess.Popen(
-        [sys.executable, "-c", "import sys; sys.stdin.buffer.read()"],
+        [sys.executable, "-c", "import sys; sys.stdin.buffer.read()", _SERVER_CLASS],
         stdin=subprocess.PIPE,
         stdout=subprocess.DEVNULL,
         stderr=subprocess.DEVNULL,
     )
 
 
+def _spawn_stubborn_sleeper() -> "subprocess.Popen":
+    """A sleeper that ignores SIGTERM, standing in for a server hanging in shutdown. It
+    prints one line once its handler is installed so tests do not signal it too early."""
+    proc = subprocess.Popen(
+        [
+            sys.executable,
+            "-c",
+            "import signal, time\n"
+            "signal.signal(signal.SIGTERM, signal.SIG_IGN)\n"
+            "print('ready', flush=True)\n"
+            "time.sleep(300)",

Review Comment:
   Sleeping in tests could lead to flakiness and slow tests. Is it possible to use explicit synchronization instead?



##########
python/pyspark/sql/connect/local_server_pool.py:
##########
@@ -107,13 +113,146 @@ def resolved(command: str) -> str:
 # layers above, which measure a member's age as ``time.time() - created``.
 _MAX_CREATED = 253402300799
 
+# A retired server still alive this long after retirement is hard-killed. After the give-up
+# age, tracking is removed only once the process is gone, replaced, or successfully signalled.
+_RETIRE_KILL_AFTER = 30
+_RETIRE_GIVE_UP = 600
+_POOL_STATE_TEMP_PREFIX = ".pool-state-"
+
+
+def _positive_pid(value: Any) -> Optional[int]:

Review Comment:
   can we move these helper functions and the above magic numbers to a class that makes sense, for better encapsulation?



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