Yicong-Huang commented on code in PR #5027:
URL: https://github.com/apache/texera/pull/5027#discussion_r3239222754
##########
amber/src/main/python/core/runnables/heartbeat.py:
##########
@@ -79,22 +79,26 @@ def run(self) -> None:
def _check_heartbeat(self) -> bool:
"""
- Attempt to connect to JVM on the specific port. If succeeds, it means
the
- socket is still available and the JVM is still alive. Otherwise, the
JVM
- might have been gone.
+ Attempt to connect to the JVM port. If connection failure, then JVM is
dead, or if connection success
+ then JVM is alive even if close() raises. Logs on connection failure
and on close error.
Review Comment:
Grammar nit: `If connection failure ... or if connection success` reads
awkwardly. Suggest: `Returns True iff socket.create_connection succeeds; a
close() failure after a successful connect is logged but does not flip the
result.`
##########
amber/src/test/python/core/runnables/test_heartbeat.py:
##########
@@ -79,20 +79,15 @@ def
test_returns_false_when_socket_connection_times_out(self):
):
assert hb._check_heartbeat() is False
- def test_returns_false_when_socket_close_raises(self):
- # Pins the false-negative path: connect succeeds but the subsequent
- # close() throws (e.g. broken pipe on a half-open socket). The bare
- # `except Exception` in _check_heartbeat() catches it and reports
- # "server down", and a regression that narrows that handler would be
- # caught here.
+ def
test_returns_true_when_connection_succeeds_but_socket_close_raises(self):
hb = make_heartbeat()
fake_sock = MagicMock()
fake_sock.close.side_effect = OSError("close failed")
with patch(
"core.runnables.heartbeat.socket.create_connection",
return_value=fake_sock,
):
- assert hb._check_heartbeat() is False
+ assert hb._check_heartbeat() is True
Review Comment:
Also assert `fake_sock.close.assert_called_once()` so a regression that
swaps the order (or drops the close call entirely) is caught here, not just at
runtime FD leaks.
--
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]
