Re: [PR] [SPARK-59032][CORE] Apply gRPC keepalive so a wedged Connect stream recovers instead of hanging [spark-connect-rust]

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

HyukjinKwon commented on PR #58:
URL: https://github.com/apache/spark-connect-rust/pull/58#issuecomment-5434046573

   Thanks @viirya — thorough review, all fair. Addressed in the latest commit (866acf0):
   
   - **keepalive timeout coercion.** Fixed the real bug: `.max(0)` is gone. The logic now lives in a validated `ChannelBuilder::keepalive()` that returns `None` (never a zero-duration) when the interval *or* timeout is non-positive — so `grpc_keepalive_timeout_ms=-1` disables keepalive, consistent with how a `-1` interval already behaves, instead of producing an immediately-expiring PING. Added a unit test for exactly the (valid-interval, `-1`-timeout) combination.
   - **max message length (128 MiB parsed but never applied).** Agreed it's the same bug-class and a real >4 MiB decode hazard. Filed as a follow-up: **SPARK-59037**, so it isn't rediscovered as a mystery decode error.
   - **diagnostic layer in the description.** Folded the faulthandler watchdog + `_dump_server_jvm` into the PR description explicitly. Also tightened all three: `found[-1]` instead of `[0]`, raised the jstack cap 20k→60k (the first run truncated exactly the execute/PythonRunner threads), a comment that the JVM dump is only unambiguous at `--jobs 1`, and a try/except around the dump-file open so an unwritable `RUST_HANG_DUMP` can't fail plugin import.
   - **two root causes / framing.** Fully agree. Reframed both the PR and SPARK-59032: keepalive makes the symptom *recoverable* but does **not** explain "server produced nothing while the reference got an error in <1s," so the ticket stays open for the true cause (the added jstack targets exactly that). Noted the `FLAKY_FILES` retry is a 360s-cap-per-attempt trade — coverage over speed.
   - **smaller things.** Removed the duplicated lazy-connect comment, reverted the `skipped_file` `"env"` string back to a bool, switched to inline `{e}` captures, and trimmed the keepalive comment to the *why*.
   


-- 
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