ericm-db commented on PR #57907:
URL: https://github.com/apache/spark/pull/57907#issuecomment-5360650492
@dtenedor, I addressed the second-round feedback in pushed commit
`36c787ab5d6`:
1. `_await_ready` now uses the shared `_port_open("localhost", port)`
instead of the last inlined socket probe. This completes the reachability-probe
consolidation and means transient `OSError` or `UnicodeError` failures retry
until the deadline rather than aborting launch.
2. The POSIX guard now lives in `_pid_alive` itself; off POSIX it returns
`True`, avoiding an `os.kill` call that could terminate the target rather than
probe it. `is_reusable` dropped its duplicate guard and now shares the helper
with `PoolMember.is_usable`. `test_pid_probe_is_skipped_on_windows` still
passes and now verifies the helper-level guard.
3. `test_fingerprint_includes_jvm_env` now covers all seven `_JVM_ENV_VARS`
using an independent expected list and verifies that changing each variable
changes the fingerprint. This avoids the circular coverage of the earlier
version that iterated `_JVM_ENV_VARS` directly.
4. The `_JVM_ENV_VARS` comment now documents the deliberate omission of
`PATH`: `bin/spark-class` prefers `${JAVA_HOME}/bin/java`, interpreter lookup
is already tracked indirectly through `shutil.which`, and the full `PATH` is
too volatile to fingerprint. Runs requiring a specific JDK should set
`JAVA_HOME`.
5. I corrected the PR description to say fifteen tests added at this layer
and 25 total, changed "deterministic FIFO" to "well-defined,
approximately-FIFO" with the wall-clock-ordering caveat, and linked the GitHub
Actions run for the latest commit.
6. The malformed co-author trailer is resolved: the new commit has no
`Co-authored-by` trailer and is authored solely by the PR author.
One substantive item is intentionally deferred; please confirm that you
agree with this placement:
- The `_MAX_CREATED` reap-leak follow-up belongs in the reaper
(`_reap_server` / `_kill_recorded_daemon`), which is part of the
acquisition-layer PR #57687 and is not on this branch. I plan to address it
there rather than disturb this layer's "reject a corrupt record -> `None`"
contract. Thank you for the detailed trace showing how a good PID reaches that
branch.
Thank you for the thorough second-round review.
--
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]
