[SPARK-58021][CONNECT] Generalize local Connect server launching by ericm-db · Pull Request #57684 · apache/spark

ericm-db · 2026-07-31T17:08:40Z

What changes were proposed in this pull request?

This is layer 1 of the seven-PR local Connect pool stack:

#57684 -> #57685 -> #57907 -> #57686 -> #57687 -> #57102 -> #57688

This patch extracts the reusable pieces of local Spark Connect server startup from the
persistent-server reuse path:

expose the per-user runtime directory and startup seed configuration as module helpers;
add LocalConnectServer.start() as the common lifecycle-owned launch entry point; and
let the launcher use an ephemeral port and a precomputed startup configuration when requested.

The existing reuse path now calls the same LocalConnectServer.start() method. Later layers add
filesystem storage, member claiming, lifecycle management, acquisition, SparkSession integration,
and optional JIT warmup.

Why are the changes needed?

The persistent reuse mode currently combines reusable local-server lifecycle handling with
assumptions specific to its one fixed daemon. The single-use server pool needs the same secure
configuration seeding, process launch, discovery, and readiness handling, but with independent
runtime directories and ephemeral ports. Sharing one launch path keeps those behaviors consistent
and isolates the larger feature from the already-working reuse implementation.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added focused unit coverage to
pyspark.sql.tests.connect.test_connect_local_server.LocalConnectServerReuseTests. Local validation
before the final review updates ran the then-current 15-test suite, including real daemon startup,
reuse, session isolation, and static configuration seeding:

python -m unittest -v pyspark.sql.tests.connect.test_connect_local_server

The final tree contains 21 test methods in this suite. The PR's final head passed GitHub's PySpark
Connect, aggregated test-report, and linter, license, and dependency checks.

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

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

Expose the per-user runtime directory and startup seed configuration, and let LocalConnectServer start isolated daemons with an ephemeral port and precomputed configuration. Keep persistent-server reuse on the same launch path and add focused unit coverage.

dtenedor

Review notes

Behavior-preserving extraction looks solid for the reuse path and lines up with what #57102 needs. A few notes on the seed_conf contract and coverage of the new ServerLauncher knobs — details inline.

dtenedor · 2026-07-31T18:29:27Z

-                conf.pop(k)
-        return conf
+        if self._seed_override is not None:
+            return dict(self._seed_override)


[Medium — correctness] The override path returns seed_conf verbatim and skips the key stripping that startup_seed_conf applies (spark.master, spark.connect.grpc.binding.port, spark.local.connect.*, token, etc.).

#57102 happens to call startup_seed_conf(opts) before passing the result, so it is safe today, but this API does not enforce that. A caller that passes raw builder opts as seed_conf can put conflicting keys into --properties-file.

Suggestion: always run the same strip on the override (idempotent if already sanitized), so the invariant lives in one place — or document + assert that seed_conf must already be post-startup_seed_conf.

Yeah, better to keep the invariant in one place. Pulled the strip into _strip_launcher_conf and run the override through it too. It's idempotent so #57102's path is unchanged, but now raw opts passed as a seed can't leak spark.master/port/token/spark.local.connect.* into --properties-file. startup_seed_conf uses the same helper.

dtenedor · 2026-07-31T18:29:27Z

                return sock.getsockname()[1]

-        if "SPARK_TESTING" in os.environ:
+        if self._use_ephemeral_port or "SPARK_TESTING" in os.environ:


[Medium — test coverage] This is the new production path that matters for pool attendants (they will not have SPARK_TESTING set). test_start_delegates_launch_options only asserts that ServerLauncher(...) received use_ephemeral_port=True; it mocks away the launcher, so _pick_port never runs. Under a normal test env where SPARK_TESTING is set, the flag is also a no-op.

Suggestion: add a unit test that pops SPARK_TESTING, constructs a launcher with use_ephemeral_port=True, and asserts _pick_port() takes the free-port path rather than the configured/default port. Optionally also cover use_ephemeral_port=False still honoring the configured port when testing is unset.

Added a test that pops SPARK_TESTING, sets use_ephemeral_port=True with a non-integer configured port, and asserts _pick_port() still returns a real port - the configured branch would've raised on int(...), so a clean return proves it took the free-port path. Also added one for the use_ephemeral_port=False case honoring the configured port.

dtenedor · 2026-07-31T18:29:27Z

@@ -303,20 +355,9 @@ def _seed_conf(self) -> Dict[str, Any]:
        opt-in keys. Only the run that starts the server can seed static confs; later runs
        find the JVM already warm.
        """


[Low — docs] This docstring still describes only the merge+strip path and does not mention the seed_conf override short-circuit (or that overrides are currently taken as-is). Easy to misread when implementing callers — worth updating alongside any sanitization change.

Updated it to cover both paths - the env+opts merge and the verbatim override - and that either way the result goes through _strip_launcher_conf.

dtenedor · 2026-07-31T18:29:27Z

+                },
+            )
+
+    def test_start_delegates_launch_options(self) -> None:


[Low — test coverage] Together with test_startup_seed_conf, this covers the helper and the LocalConnectServer.start wiring, but the ServerLauncher seed-override path itself is never exercised: _seed_conf() returning the override, and especially {} vs None (load-bearing for the pool attendant, which passes opts={} plus a precomputed seed_conf — empty dict must not fall through to env PYSPARK_REMOTE_INIT_CONF_*).

Suggestion: small ServerLauncher unit tests for _seed_conf / _seed_properties_file with override, None, and {}.

Added ServerLauncher tests for _seed_conf/_seed_properties_file: override (used minus stripped keys), {} (stays empty, doesn't fall through to PYSPARK_REMOTE_INIT_CONF_*), and None (env+opts merge). Empty seed yields no properties file; non-empty writes a 0600 file with the confs.

…launch knobs Centralize the launcher-managed key stripping in a new `_strip_launcher_conf` helper and run the `seed_conf` override through it, so raw builder opts passed as a seed can no longer leak `spark.master`, the binding port, the auth token, or `spark.local.connect.*` into `--properties-file`. Update the `_seed_conf` docstring to describe both the merge and the verbatim-override paths. Add unit coverage for the new `ServerLauncher` knobs: `_pick_port` taking the ephemeral free-port path outside `SPARK_TESTING` (and honoring a configured port when neither is set), and `_seed_conf` / `_seed_properties_file` for the override, `None`, and load-bearing empty-dict cases. Co-authored-by: Isaac

uros-b · 2026-07-31T21:30:25Z

Thank you @ericm-db! cc @tgravescs

HyukjinKwon

0 blocking, 0 non-blocking, 0 nits.
Clean, behavior-preserving extraction; all reviewer feedback already addressed. No issues.

Verification

Confirmed the reuse path is unchanged; runtime_dir() is a verbatim hoist (same 0700 makedirs + chmod + error class); _strip_launcher_conf is idempotent and now guards the override too, so launcher-managed keys cannot reach --properties-file even from raw opts; startup_seed_conf mirrors the canonical session.py INIT_CONF last-wins loop. All 4 of dtenedor's review points are resolved in head. Build/test: python/run-tests --testnames 'pyspark.sql.tests.connect.test_connect_local_server'.

ericm-db · 2026-08-05T23:01:31Z

@dtenedor @HyukjinKwon I think this is ready to merge if the prior comments were addressed adequately

dtenedor · 2026-08-10T16:56:30Z

LGTM, merging to master and 4.x

### What changes were proposed in this pull request? This is layer 1 of the six-PR local Connect pool stack: #57684 -> #57685 -> #57686 -> #57687 -> #57102 -> #57688 This patch extracts the reusable pieces of local Spark Connect server startup from the persistent-server reuse path: - expose the per-user runtime directory and startup seed configuration as module helpers; - add `LocalConnectServer.start()` as the common lifecycle-owned launch entry point; and - let the launcher use an ephemeral port and a precomputed startup configuration when requested. The existing reuse path now calls the same `LocalConnectServer.start()` method. Focused tests cover seed configuration and delegation of the new launch options. ### Why are the changes needed? The persistent reuse mode currently combines reusable local-server lifecycle handling with assumptions specific to its one fixed daemon. The single-use server pool needs the same secure configuration seeding, process launch, discovery, and readiness handling, but with independent runtime directories and ephemeral ports. Sharing one launch path keeps those behaviors consistent and isolates the larger feature from the already-working reuse implementation. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added focused unit coverage to `pyspark.sql.tests.connect.test_connect_local_server.LocalConnectServerReuseTests` and ran the full suite, including real daemon startup, reuse, session isolation, and static configuration seeding: ```bash python -m unittest -v pyspark.sql.tests.connect.test_connect_local_server ``` All 15 tests passed. Ruff check, Ruff format check, and `git diff --check` passed. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5) Closes #57684 from ericm-db/local-connect-pool-refactor. Authored-by: Eric Marnadi <eric.marnadi@databricks.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com> (cherry picked from commit 343fbc3) Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

dtenedor · 2026-08-10T17:04:01Z

Merge Summary:

merged into master 343fbc3
merged into branch-4.x 11d0c54

Posted by merge_spark_pr.py

### What changes were proposed in this pull request? This is layer 2 of the seven-PR local Connect pool stack: #57684 -> #57685 -> #57907 -> #57686 -> #57687 -> #57102 -> #57688 The review unit introduced here is commit `23f806b64ad`. This layer adds the filesystem-backed storage foundation for pool members: - stable state-file paths keyed by member ID and per-member directories; - an overridable private pool directory under the per-user runtime directory; - a per-pool cross-process POSIX file lock; - private directory, lock-file, and JSON state-file permissions; and - locked helpers for listing, reading, writing, renaming, and removing member state. Member validation, compatibility fingerprints, and atomic claiming are isolated in #57907. Process lifecycle, acquisition, SparkSession integration, and JIT warmup remain in later PRs. ### Why are the changes needed? The pool needs a small, independently reviewable state model before adding compatibility checks, claiming, and process supervision. Keeping this layer limited to path layout, locking, and state file access makes its filesystem and concurrency contract reviewable on its own. ### Does this PR introduce _any_ user-facing change? No. The storage model is internal and is not wired into SparkSession in this layer. ### How was this patch tested? Added three focused tests covering directory selection, private permissions and malformed JSON, and cross-process lock contention. ```bash python/run-tests --testnames pyspark.sql.tests.connect.test_connect_local_server_pool ``` These cases passed on Python 3.11 as part of the combined suite before the stack was split. The rebuilt commit passed `git diff --check`, Python AST parsing, and changed-line ASCII and 100-column checks. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5) Closes #57685 from ericm-db/local-connect-pool-storage. Authored-by: Eric Marnadi <eric.marnadi@databricks.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

### What changes were proposed in this pull request? This is layer 2 of the seven-PR local Connect pool stack: #57684 -> #57685 -> #57907 -> #57686 -> #57687 -> #57102 -> #57688 The review unit introduced here is commit `23f806b64ad`. This layer adds the filesystem-backed storage foundation for pool members: - stable state-file paths keyed by member ID and per-member directories; - an overridable private pool directory under the per-user runtime directory; - a per-pool cross-process POSIX file lock; - private directory, lock-file, and JSON state-file permissions; and - locked helpers for listing, reading, writing, renaming, and removing member state. Member validation, compatibility fingerprints, and atomic claiming are isolated in #57907. Process lifecycle, acquisition, SparkSession integration, and JIT warmup remain in later PRs. ### Why are the changes needed? The pool needs a small, independently reviewable state model before adding compatibility checks, claiming, and process supervision. Keeping this layer limited to path layout, locking, and state file access makes its filesystem and concurrency contract reviewable on its own. ### Does this PR introduce _any_ user-facing change? No. The storage model is internal and is not wired into SparkSession in this layer. ### How was this patch tested? Added three focused tests covering directory selection, private permissions and malformed JSON, and cross-process lock contention. ```bash python/run-tests --testnames pyspark.sql.tests.connect.test_connect_local_server_pool ``` These cases passed on Python 3.11 as part of the combined suite before the stack was split. The rebuilt commit passed `git diff --check`, Python AST parsing, and changed-line ASCII and 100-column checks. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5) Closes #57685 from ericm-db/local-connect-pool-storage. Authored-by: Eric Marnadi <eric.marnadi@databricks.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com> (cherry picked from commit 001e89c) Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

This comment was marked as resolved.

Sign in to view

dtenedor reviewed Jul 31, 2026

View reviewed changes

ericm-db requested a review from dtenedor July 31, 2026 21:06

uros-b approved these changes Jul 31, 2026

View reviewed changes

HyukjinKwon reviewed Aug 5, 2026

View reviewed changes

ericm-db requested a review from HyukjinKwon August 5, 2026 23:00

dtenedor approved these changes Aug 10, 2026

View reviewed changes

dtenedor closed this in 343fbc3 Aug 10, 2026

ericm-db mentioned this pull request Aug 10, 2026

[SPARK-58021][CONNECT] Add local server pool member claiming #57907

Open

Uh oh!

Conversation

ericm-db commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

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?

Uh oh!

This comment was marked as resolved.

Uh oh!

dtenedor left a comment

Choose a reason for hiding this comment

Review notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uros-b commented Jul 31, 2026

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Verification

Uh oh!

ericm-db commented Aug 5, 2026

Uh oh!

dtenedor commented Aug 10, 2026

Uh oh!

dtenedor commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericm-db commented Jul 31, 2026 •

edited

Loading