Re: [PR] [SPARK-57793][CONNECT] Add PathAwareChannelBuilder for SparkConnect python client [spark]

["viirya (via GitHub)" <gi...@apache.org>]

viirya commented on PR #56933:
URL: https://github.com/apache/spark/pull/56933#issuecomment-5257771238

   Following up on my earlier comment about the three routing shapes — I went and actually ran the experiments this time, both for option A (this PR) and option C (`:authority`), so here's what I found on the record.
   
   **Option A — the prefix on `:path` is a two-sided contract, and I confirmed the failure mode.**
   Because the gRPC method name *is* the HTTP/2 `:path`, prepending `/sparkConnect` makes the wire path `/sparkConnect/spark.connect.SparkConnectService/AnalyzePlan`. I verified against current Spark master (in-process `SparkConnectService`, a raw gRPC channel carrying the prefixed path) that the server returns `UNIMPLEMENTED` for the prefixed method and serves the clean one. So the driver only works if the ingress strips the prefix back off — correctness is split across two sides (client prepends, ingress strips), and a client-only misconfiguration fails with a confusing `UNIMPLEMENTED`.
   
   To be precise about the ingress rewrite: I set up nginx with `rewrite ^/sparkConnect/(.*) /$1 break;` + `grpc_pass` and it **did** strip the prefix correctly, for both unary and streaming RPCs (nginx 1.31.3) — so a working setup like yours doesn't surprise me. My concern is narrower than "it doesn't work": it's that this rewrite is not officially supported and has no stability contract. `grpc_pass` takes an address, not a URI, and its docs don't cover rewriting `:path`; the official ingress-nginx gRPC example mounts the service at `/` with no `rewrite-target`; and there's no e2e coverage for the gRPC + rewrite combination. It may work on one nginx build and change on another — it's a rewrite the operator owns and maintains.
   
   **Option C — `:authority` — I ran it end-to-end and it works, including under TLS.**
   Topology: **current** PySpark client (no code change) → nginx → Spark Connect Gateway → a real Spark Connect driver. On the client I set the `grpc.default_authority` channel option; on nginx I route with a plain `server_name`. A real `spark.sql()` runs through the whole chain, and `:path` is never touched. Setting the authority to a different value returns 404 from nginx, confirming routing keys on `:authority`.
   
   ```python
   from pyspark.sql.connect.session import SparkSession
   from pyspark.sql.connect.client import DefaultChannelBuilder
   
   cb = DefaultChannelBuilder("sc://cluster1.xyz.com:443")
   cb.setChannelOption("grpc.default_authority", "sparkConnect")
   spark = SparkSession.builder.channelBuilder(cb).getOrCreate()
   ```
   ```nginx
   server {
       listen 443 ssl;
       http2 on;
       server_name sparkConnect;              # matches :authority
       location / { grpc_pass grpc://spark-connect-gateway:15003; }
   }
   ```
   
   On the TLS wrinkle I raised earlier for C: it's resolvable. Overriding `:authority` alone does fail cert verification (gRPC uses the authority as the TLS verification name by default), but keeping the two names separate fixes it — I set `grpc.ssl_target_name_override` to the gateway's real hostname (so the cert only needs to cover that) and `grpc.default_authority` to the routing tag, and the handshake + routing both work. I confirmed this with grpcio.
   
   I do have a real reservation about the direction: option A grows the client toward polluting `:path` and depending on an ingress rewrite that has no support contract to undo it, and that's a shape I'm not comfortable baking into the client. Option C (the one the gRPC maintainers recommended in grpc/grpc#14900) reaches the same goal with no client code change and without touching the request at all — given that, I'm not convinced adding A to the client is the right trade-off.
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org