refactor(submit): drop spark-submit retry loop; confirm raises on failure
confirm_submit_job no longer retries on SparkSubmitError. A failed
single attempt is recorded as-is:
- pending.status = 'FAILED' with the error message in pending.error
- the function re-raises the SparkSubmitError so the caller (and the
agent) sees the failure immediately
- the agent can call get_pending_job to see the persisted FAILED
state, including the captured error
The 'retry' code path was hiding real failures behind transient
recovery: a spark-submit that died because the script is broken looks
the same as one that died because YARN was momentarily unreachable.
Without retries, the agent gets a clean FAILED state and a clear
exception to act on, instead of a delayed, ambiguous result.
Manual recovery is still possible: re-calling confirm_submit_job on a
FAILED pending resets it to PENDING and runs a single fresh attempt
(see test_confirm_resets_failed_then_succeeds). The reset path is
useful for 'I fixed the script, try again' flows; it just is no longer
the default on every failure.
Removed:
- the for-attempt loop and time.sleep in submit.py
- import time in submit.py and test_submit_tool.py
- confirm_max_retries and confirm_retry_delay_seconds from
common/config.py (env vars, snapshot, reload)
- 2 retry tests (test_confirm_retries_spark_submit_failure_then_succeeds,
test_confirm_exhausts_retries_and_sets_failed)
- test_confirm_marks_failed_on_spark_submit_error updated to assert
the new single-attempt + raise contract
- test_confirm_resets_failed_and_retries renamed to
test_confirm_resets_failed_then_succeeds (no more retry loop to
exercise)
Full suite 244 passed (3 fewer than before, matching the removed
tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,7 +1,6 @@
|
||||
# coding=utf-8
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -338,6 +337,8 @@ def test_confirm_refuses_non_pending_status(monkeypatch, real_script):
|
||||
|
||||
|
||||
def test_confirm_marks_failed_on_spark_submit_error(monkeypatch, real_script):
|
||||
"""confirm_submit_job makes a single spark-submit attempt; on failure
|
||||
it sets pending.status=FAILED and re-raises the SparkSubmitError."""
|
||||
submit.prepare_submit_job(
|
||||
connection="prod",
|
||||
script_path=str(real_script),
|
||||
@@ -348,71 +349,20 @@ def test_confirm_marks_failed_on_spark_submit_error(monkeypatch, real_script):
|
||||
app_name="test-app",
|
||||
)
|
||||
pid = _last_pending_id()
|
||||
call_count = [0]
|
||||
def _raise(_cmd):
|
||||
call_count[0] += 1
|
||||
raise SparkSubmitError("boom")
|
||||
monkeypatch.setattr(submit, "run_spark_submit", _raise)
|
||||
settings.confirm_max_retries = 0
|
||||
with pytest.raises(SparkSubmitError):
|
||||
with pytest.raises(SparkSubmitError, match="boom"):
|
||||
submit.confirm_submit_job(pending_id=pid)
|
||||
assert call_count[0] == 1 # single attempt, no retry
|
||||
p = submit.pending_store.get(pid)
|
||||
assert p.status == "FAILED"
|
||||
assert "boom" in (p.error or "")
|
||||
|
||||
|
||||
# --- confirm retry / idempotency ---
|
||||
|
||||
def test_confirm_retries_spark_submit_failure_then_succeeds(monkeypatch, real_script):
|
||||
monkeypatch.setattr(time, "sleep", lambda _s: None)
|
||||
settings.confirm_max_retries = 2
|
||||
submit.prepare_submit_job(
|
||||
connection="prod",
|
||||
script_path=str(real_script),
|
||||
queue="default",
|
||||
executor_memory="2G",
|
||||
executor_cores=2,
|
||||
num_executors=2,
|
||||
app_name="test-app",
|
||||
)
|
||||
pid = _last_pending_id()
|
||||
calls = []
|
||||
def _maybe_fail(cmd):
|
||||
calls.append(cmd)
|
||||
if len(calls) < 3:
|
||||
raise SparkSubmitError("transient")
|
||||
return _spark_proc()
|
||||
monkeypatch.setattr(submit, "run_spark_submit", _maybe_fail)
|
||||
result = submit.confirm_submit_job(pending_id=pid)
|
||||
assert result.application_id == "application_17400000001"
|
||||
assert len(calls) == 3
|
||||
p = submit.pending_store.get(pid)
|
||||
assert p.status == "SUBMITTED"
|
||||
|
||||
|
||||
def test_confirm_exhausts_retries_and_sets_failed(monkeypatch, real_script):
|
||||
monkeypatch.setattr(time, "sleep", lambda _s: None)
|
||||
settings.confirm_max_retries = 2
|
||||
submit.prepare_submit_job(
|
||||
connection="prod",
|
||||
script_path=str(real_script),
|
||||
queue="default",
|
||||
executor_memory="2G",
|
||||
executor_cores=2,
|
||||
num_executors=2,
|
||||
app_name="test-app",
|
||||
)
|
||||
pid = _last_pending_id()
|
||||
calls = []
|
||||
def _always_fail(cmd):
|
||||
calls.append(cmd)
|
||||
raise SparkSubmitError("persistent")
|
||||
monkeypatch.setattr(submit, "run_spark_submit", _always_fail)
|
||||
with pytest.raises(SparkSubmitError, match="persistent"):
|
||||
submit.confirm_submit_job(pending_id=pid)
|
||||
assert len(calls) == 3
|
||||
p = submit.pending_store.get(pid)
|
||||
assert p.status == "FAILED"
|
||||
assert "persistent" in (p.error or "")
|
||||
|
||||
# --- confirm idempotency / manual retry ---
|
||||
|
||||
def test_confirm_idempotent_when_submitted(monkeypatch, real_script):
|
||||
submit.prepare_submit_job(
|
||||
@@ -440,8 +390,10 @@ def test_confirm_idempotent_when_submitted(monkeypatch, real_script):
|
||||
assert second.tracking_url == first.tracking_url
|
||||
|
||||
|
||||
def test_confirm_resets_failed_and_retries(monkeypatch, real_script):
|
||||
monkeypatch.setattr(time, "sleep", lambda _s: None)
|
||||
def test_confirm_resets_failed_then_succeeds(monkeypatch, real_script):
|
||||
"""A FAILED pending can be re-confirmed after the user fixes the
|
||||
underlying issue. confirm_submit_job resets FAILED -> PENDING,
|
||||
then makes a single fresh attempt (no automatic retry loop)."""
|
||||
submit.prepare_submit_job(
|
||||
connection="prod",
|
||||
script_path=str(real_script),
|
||||
@@ -457,15 +409,20 @@ def test_confirm_resets_failed_and_retries(monkeypatch, real_script):
|
||||
"run_spark_submit",
|
||||
lambda _cmd: (_ for _ in ()).throw(SparkSubmitError("boom")),
|
||||
)
|
||||
with pytest.raises(SparkSubmitError):
|
||||
with pytest.raises(SparkSubmitError, match="boom"):
|
||||
submit.confirm_submit_job(pending_id=pid)
|
||||
p = submit.pending_store.get(pid)
|
||||
assert p.status == "FAILED"
|
||||
assert "boom" in (p.error or "")
|
||||
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda _cmd: _spark_proc())
|
||||
call_count = [0]
|
||||
def _counting_run(cmd):
|
||||
call_count[0] += 1
|
||||
return _spark_proc()
|
||||
monkeypatch.setattr(submit, "run_spark_submit", _counting_run)
|
||||
result = submit.confirm_submit_job(pending_id=pid)
|
||||
assert result.application_id == "application_17400000001"
|
||||
assert call_count[0] == 1
|
||||
p = submit.pending_store.get(pid)
|
||||
assert p.status == "SUBMITTED"
|
||||
assert p.error is None
|
||||
|
||||
Reference in New Issue
Block a user