cloud-fan commented on PR #57854: URL: https://github.com/apache/spark/pull/57854#issuecomment-5236369064
Thanks for the detailed review. I addressed the actionable points in c419b45bb5c: 1. The process check now has an explicit inconclusive result. On a failed or timed-out `ps` probe, `--stop` preserves discovery and the daemon PID file so the user can retry, and reports that it could not verify the server. 2. I left the `_await_ready` timeout cleanup unchanged. That PID comes directly from the daemon launched in the same startup attempt, so the reuse risk is negligible; applying the same inconclusive handling there could instead leave a startup-time daemon with no discovery record. 3. Agreed that the `ps`/`kill` TOCTOU window remains. Avoiding it would require a platform-specific mechanism such as pidfd, so this guard is intentionally a mitigation. 4. Agreed on the IPv4 limitation. I left `localhost` in discovery because the Connect client currently uses that literal hostname to select gRPC local credentials; changing it to `127.0.0.1` would require a broader client-channel change. 5. The module documentation now states that the server always binds IPv4 loopback and overrides any configured binding address. 6. The launcher test now sets `SPARK_HOME` explicitly and verifies the single expected script-existence check, avoiding interaction with `_find_spark_home()`. 7. I updated the PR testing section with the actual commands. `build/sbt -Phive package` and all 16 tests in `pyspark.sql.tests.connect.test_connect_local_server` pass. Thanks again! -- 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]
