dtenedor commented on code in PR #57684:
URL: https://github.com/apache/spark/pull/57684#discussion_r3692554785
##########
python/pyspark/sql/connect/local_server.py:
##########
@@ -269,20 +321,20 @@ def _token(self) -> str:
)
def _pick_port(self) -> int:
- """Under SPARK_TESTING always use an OS-assigned free port so suites
can run in
- parallel; otherwise honor the configured/default port, falling back to
a free one if
- another process holds it. (A live stale server of ours also holds the
port, but that
- start fails later at spark-daemon.sh's pid-file check regardless of
port.) The sbin
- script cannot report an ephemeral port back, so the free port is
picked and released
- here, with a small race until the server binds it.
+ """Use an OS-assigned free port when requested or under SPARK_TESTING
so suites can
+ run in parallel. Otherwise honor the configured/default port, falling
back to a free
+ one if another process holds it. (A live stale server of ours also
holds the port, but
+ that start fails later at spark-daemon.sh's pid-file check regardless
of port.) The
+ sbin script cannot report an ephemeral port back, so the free port is
picked and
+ released here, with a small race until the server binds it.
"""
def free_port() -> int:
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
sock.bind(("localhost", 0))
return sock.getsockname()[1]
- if "SPARK_TESTING" in os.environ:
+ if self._use_ephemeral_port or "SPARK_TESTING" in os.environ:
Review 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.
##########
python/pyspark/sql/connect/local_server.py:
##########
@@ -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.
"""
- conf: Dict[str, Any] = {}
- for i in range(int(os.environ.get("PYSPARK_REMOTE_INIT_CONF_LEN",
"0"))):
- conf =
json.loads(os.environ["PYSPARK_REMOTE_INIT_CONF_{}".format(i)])
- conf.update(self._opts)
- for k in list(conf):
- if k in (
- "spark.remote",
- "spark.api.mode",
- "spark.master",
- "spark.connect.authenticate.token",
- "spark.connect.grpc.binding.port",
- ) or k.startswith("spark.local.connect."):
- conf.pop(k)
- return conf
+ if self._seed_override is not None:
+ return dict(self._seed_override)
Review 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`.
##########
python/pyspark/sql/tests/connect/test_connect_local_server.py:
##########
@@ -114,6 +114,65 @@ def test_discovery_location(self) -> None:
self.assertIn("spark-connect-{}".format(getpass.getuser()),
default.directory)
self.assertEqual(os.stat(default.directory).st_mode & 0o777, 0o700)
+ def test_startup_seed_conf(self) -> None:
+ from unittest import mock
+
+ initial = {
+ "spark.sql.shuffle.partitions": "8",
+ "spark.master": "local[1]",
+ }
+ opts = {
+ "spark.sql.warehouse.dir": os.path.join(self._tmpdir, "warehouse"),
+ "spark.local.connect.reuse": "true",
+ "spark.connect.grpc.binding.port": "0",
+ }
+ env = {
+ "PYSPARK_REMOTE_INIT_CONF_LEN": "1",
+ "PYSPARK_REMOTE_INIT_CONF_0": json.dumps(initial),
+ }
+ with mock.patch.dict(os.environ, env):
+ self.assertEqual(
+ local_server.startup_seed_conf(opts),
+ {
+ "spark.sql.shuffle.partitions": "8",
+ "spark.sql.warehouse.dir": opts["spark.sql.warehouse.dir"],
+ },
+ )
+
+ def test_start_delegates_launch_options(self) -> None:
Review 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 `{}`.
##########
python/pyspark/sql/connect/local_server.py:
##########
@@ -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.
"""
Review 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.
--
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]
