HyukjinKwon commented on PR #64:
URL:
https://github.com/apache/spark-connect-rust/pull/64#issuecomment-5448906340
Thanks for the rigorous review — I verified each point against the reference
source (`pyspark/sql/connect/client/core.py`) and you're right throughout, so I
took your recommended route in 13ad5e1:
**No reference counterpart / metadata leak.** Confirmed:
`GRPC_DEFAULT_OPTIONS` (core.py:238-250) sets
`grpc.max_{send,receive}_message_length` from `GRPC_MAX_MESSAGE_LENGTH_DEFAULT`
(128 MiB) *unconditionally*; there is no `grpc_max_message_size` connection
param, and the reference's param allow-list and metadata-exclusion list carry
only the 5 base + 4 keepalive keys. Our `RESERVED` omission would indeed have
sent a `grpc_max_message_size` header on every RPC.
Fix: dropped `PARAM_GRPC_MAX_MESSAGE_SIZE` and
`ChannelBuilder::max_message_size()` (with its `<= 0` fallback and unit test),
and now apply `GRPC_MAX_MESSAGE_LENGTH_DEFAULT` directly to the stub and the
three raw `Grpc` calls. That fixes the 4 MiB bug, matches the reference
exactly, needs no `RESERVED` update, and removes the leak — so the
metadata-absence test is moot. `channel.rs` now has no net change vs master
(the const was already declared there).
**Doc comment / `int_param(... as i64)` round-trip.** Both gone with the
method; the inline comment at the apply site now points to the reference's
`GRPC_DEFAULT_OPTIONS` rather than a nonexistent "reference `ChannelBuilder`"
method.
**End-to-end >4 MiB decode.** Agreed it's not unit-testable here; the path
is now mechanically identical to the reference (unconditional 128 MiB cap on
the same stub + raw paths), so it's as verified as the reference's own
handling. I'll confirm a parity test returns a >4 MiB batch to close the loop.
If per-connection configurability is wanted later, I'll do it as a separate
change using upstream's grpcio option names, ideally landing upstream first so
the clients don't diverge.
--
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]
