dbtsai commented on PR #56907:
URL: https://github.com/apache/spark/pull/56907#issuecomment-4848926607
## Review feedback
Two issues in the persistent local Connect server path that are worth
addressing before merge.
### 1. Concurrent first-time startup can fail spuriously
(`python/pyspark/sql/connect/session.py:1379`)
In non-test mode `_start_persistent_local_connect_server` requests the fixed
default port 15002 (`session.py:1336-1338`), and the wait loop hard-fails the
moment the spawned daemon exits:
```python
exit_code = proc.poll()
if exit_code is not None:
raise
PySparkRuntimeError(errorClass="LOCAL_CONNECT_SERVER_START_FAILED", ...)
```
If two opted-in processes start with no discovery file yet (parallel
workers, an IDE spawning processes, several dev scripts), both spawn a daemon
on 15002; one wins and writes discovery, the other's JVM can't bind and exits.
The losing client then raises `LOCAL_CONNECT_SERVER_START_FAILED` immediately
even though a reusable server is now available. The same port-collision
mechanism also bites after a version upgrade:
`_local_connect_server_is_reusable` rejects the stale server on version
mismatch (`session.py:1292`), then we try to start a new one on 15002 that the
old process still holds.
There is no lock around discovery creation and no reuse-the-winner retry on
child exit. Suggested fix: before raising on `exit_code is not None`, do a
final `_read_local_connect_discovery()` / `_local_connect_server_is_reusable()`
check and reconnect if a usable server is now up; ideally also hold a file lock
spanning discovery creation so only one process starts a server. (Note this is
masked in tests because `SPARK_TESTING` forces an ephemeral port 0.)
### 2. The daemon drops most builder options on first startup
(`python/pyspark/sql/connect/local_server.py:140`)
The in-process path `_start_connect_server` (`session.py:1190-1234`) merges
the full `opts` into the `SparkConf` before `SparkContext.getOrCreate(conf)`,
so static/server-side settings (warehouse dir, app name, catalog configs,
`spark.jars`/packages, driver settings, etc.) take effect. The persistent path
only forwards `--master`, `--port`, `--token`, `--idle-timeout`
(`session.py:1342-1355`), and `local_server.py:140-151` builds the JVM with
just master / plugins / port / artifact-isolation / token. The remaining `opts`
are dropped.
After connecting, the client's `_apply_options` (`session.py:204-228`) can
only apply runtime SQL confs per-session via `conf._set_all(...)` — it cannot
establish static confs on an already-running JVM. So
`.remote("local[*]").config("spark.sql.warehouse.dir", ...)` (and
jars/packages/catalog/app-name) silently has no effect on first startup,
diverging from the in-process behavior.
Caveat: a *shared* persistent server fundamentally can't honor per-client
static confs for the second+ client (the JVM is already warm), so this promise
is inherently limited. But at minimum the first client's relevant startup confs
should seed the daemon so first-run behavior matches the in-process path.
--
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]
