[SPARK-58021][CONNECT] Generalize local Connect server launching - #57684
[SPARK-58021][CONNECT] Generalize local Connect server launching#57684ericm-db wants to merge 2 commits into
Conversation
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| conf.pop(k) | ||
| return conf | ||
| if self._seed_override is not None: | ||
| return dict(self._seed_override) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| return sock.getsockname()[1] | ||
|
|
||
| if "SPARK_TESTING" in os.environ: | ||
| if self._use_ephemeral_port or "SPARK_TESTING" in os.environ: |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| @@ -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. | |||
| """ | |||
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| }, | ||
| ) | ||
|
|
||
| def test_start_delegates_launch_options(self) -> None: |
There was a problem hiding this comment.
[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 {}.
There was a problem hiding this comment.
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
|
Thank you @ericm-db! cc @tgravescs |
3 participants
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:
LocalConnectServer.start()as the common lifecycle-owned launch entry point; andThe 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.LocalConnectServerReuseTestsand ran the full suite, including real daemon startup, reuse, session isolation, and static configuration seeding:All 15 tests passed. Ruff check, Ruff format check, and
git diff --checkpassed.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5)