kaxil commented on code in PR #70183:
URL: https://github.com/apache/airflow/pull/70183#discussion_r3624725236
##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1773,6 +1778,42 @@ def
test_yarn_status_tracking_fails_on_failed_state_with_undefined_final_status(
mock_sleep.assert_not_called()
+ @patch("airflow.providers.apache.spark.hooks.spark_submit.time.sleep")
+ @patch("airflow.providers.apache.spark.hooks.spark_submit.requests.get")
+ def test_yarn_status_tracking_includes_diagnostics_on_state_failure(self,
mock_get, mock_sleep):
+ """RM state FAILED/KILLED -> raised message includes the RM's
diagnostics field."""
+ mock_get.return_value = self._rm_status_resp(
+ "KILLED", diagnostics="Application application_1700000000000_0001
was killed by user root"
Review Comment:
This test is named `..._on_state_failure` and the docstring says "RM state
FAILED/KILLED", but `_rm_status_resp("KILLED", ...)` sets `finalStatus=KILLED`
with `state` defaulting to `FINISHED`. That lands in the `final_status in
_YARN_FINAL_FAILURES` branch -- the same one
`test_yarn_status_tracking_includes_diagnostics_on_final_status_failure`
already covers. The `state in _YARN_FINAL_FAILURES` raise, where you added
`{diagnostics_suffix}` to the `ended with state:` message, never actually gets
exercised. Passing `state="KILLED"` (or `state="FAILED"`) here would cover that
branch.
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -933,16 +933,18 @@ def _start_yarn_application_status_tracking(self,
application_id: str) -> None:
elif poll_count % heartbeat_interval == 0:
self.log.info("YARN application %s is still %s",
application_id, state)
+ diagnostics_suffix = f"\nDiagnostics: {diagnostics}" if
diagnostics else ""
if state in self._YARN_FINAL_FAILURES:
raise RuntimeError(
f"YARN application {application_id} ended with state:
{state}, "
- f"final status: {final_status}"
+ f"final status: {final_status}{diagnostics_suffix}"
)
if final_status == self._YARN_FINAL_SUCCESS:
return
if final_status in self._YARN_FINAL_FAILURES:
raise RuntimeError(
f"YARN application {application_id} ended with final
status: {final_status}"
+ f"{diagnostics_suffix}"
)
if final_status != self._YARN_FINAL_UNDEFINED:
Review Comment:
This "returned unexpected final status" raise doesn't append
`diagnostics_suffix` like the two failure raises above it. YARN's
`FinalApplicationStatus` includes `ENDED`, which would fall through to here,
and it's arguably where diagnostics helps most since the final status itself is
unrecognized. Intentional, or worth appending the suffix here too?
--
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]
