viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-4860963092
Thanks for the update — the two issues from the earlier round (IPv6 handling
and the path port being lost with the trailing-slash `/;params` form) are both
fixed correctly in the current revision, with tests covering them.
A few more issues from a closer pass, roughly in order of importance:
**1. The 443-implies-TLS rule doesn't match the docstring, and one form gets
port 443 without TLS**
The docstring says "TLS is enabled implicitly when the resolved port is 443
and `;use_ssl=` was not specified", but the implication sits inside the `if
prefix != "/"` block, so:
- `PathAwareChannelBuilder("sc://gateway:443")` (no path) gets no implicit
TLS, contradicting the docstring. This matches `DefaultChannelBuilder`
behavior, so maybe it's the docstring that should be narrowed to "when a path
prefix is present".
- `PathAwareChannelBuilder("sc://host/:443")`: the port-extraction block
runs (`last_segment` is `:443`, so `self._port` becomes 443), but the prefix
then collapses to `/` and the guarded block is skipped — no TLS implication, no
interceptor. The client attempts plaintext gRPC against an HTTPS port. The port
assignment and the TLS implication should probably live under the same
condition.
**2. A `;key=value` on a non-final path segment is silently absorbed into
the prefix**
`urlparse` only strips params from the last path segment, so for
`sc://gateway/app;token=abc/driver` we get `url.params == ""` — the token is
never set, and `/app;token=abc/driver` becomes the path prefix. Auth silently
fails and the credential is sent inside the `:path` of every RPC (and lands in
ingress access logs). Since `;` can never be legal inside a route prefix under
this grammar, rejecting a prefix containing `;` with `INVALID_CONNECT_URL`
would turn this silent failure into a clear error.
**3. Consider subclassing `DefaultChannelBuilder` instead of copying it**
The scheme check in `__init__`, the params/hostname/port block of
`_extract_attributes`, the `secure`/`use_ssl`/`host`/`endpoint` properties, and
the entire `toChannel` are byte-identical copies of `DefaultChannelBuilder`.
The copies have already diverged in one place: the overridden `default_port()`
hardcodes 15002 and drops `DefaultChannelBuilder.default_port()`'s
`SPARK_TESTING` ephemeral-port resolution, so under the local test harness a
portless URL connects to the wrong port.
Since `DefaultChannelBuilder` rejects non-empty paths in `__init__`, the
subclass can pre-process the raw URL string instead: extract the path prefix
(and the optional trailing `:port`), rebuild a path-free
`sc://host:port/;params` URL, delegate to `super().__init__`, then set the SSL
default and register the interceptor. That reduces the ~120 duplicated lines to
~30 lines of genuinely new logic, and future fixes to parsing/credential
handling apply to both builders automatically.
**4. The connection-string spec and other clients**
`sql/connect/docs/client-connection-string.md` states "The path component
must be empty" and lists `sc://myhost.com:443/mypathprefix/;token=AAAAAAA` as
an explicitly invalid example — which is the exact form this PR makes valid, in
the Python client only (the Scala client's `verifyURI` still rejects non-empty
paths). Since the new form is opt-in via a separate builder and the default
`sc://` parser is unchanged, I don't think this blocks the PR, but the doc
should be updated to describe the extension (or explicitly mark it
Python-only), and a follow-up JIRA for Scala-client parity would be good so the
same connection string stays portable.
**5. Path-derived port skips validation the netloc port gets**
The netloc form goes through `urlparse`, which enforces 0-65535, but the
path form is a bare `int()`: `driver:99999` -> port 99999, `driver:-1` -> port
-1, and `driver:4_43` -> 443 via PEP 515 underscores, all silently accepted
with the bogus `:PORT` stripped from the prefix. A range/`isdigit()` check
raising `INVALID_CONNECT_URL` would make the two forms consistent.
**6. `SparkSession.Builder.channelBuilder` annotation**
`session.py` annotates `channelBuilder` (and `_channel_builder` at L129, and
`connection` at L282) as `DefaultChannelBuilder`, but `PathAwareChannelBuilder`
extends `ChannelBuilder` directly — so the usage advertised in this PR's
description is an arg-type error under mypy/pyright. Runtime is fine, but those
annotations should be widened to `ChannelBuilder` as part of this PR, since
this is the first shipped builder that isn't a `DefaultChannelBuilder`.
Minor: the class docstring's doctest is malformed (the `...` continuation
line holds an independent expression, and the expected output uses
double-quoted repr) — it's copied from `DefaultChannelBuilder`'s existing
docstring and never collected, but new code needn't repeat it. The four
`intercept_*` methods with identical bodies could also collapse into one
implementation aliased four times.
--
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]
