AlexanderBLR commented on code in PR #71380:
URL: https://github.com/apache/airflow/pull/71380#discussion_r3763885922
##########
providers/amazon/src/airflow/providers/amazon/aws/executors/batch/batch_executor.py:
##########
@@ -268,11 +268,30 @@ def sync_running_jobs(self):
self.log.debug("Active Workers: %s", describe_job_response)
for job in describe_job_response:
- if job.get_job_state() == State.FAILED:
- self._handle_failed_job(job)
- elif job.get_job_state() == State.SUCCESS:
- workload_key = self.active_workers.pop_by_id(job.job_id)
- self.success(workload_key)
+ try:
+ if job.get_job_state() == State.FAILED:
+ self._handle_failed_job(job)
+ elif job.get_job_state() == State.SUCCESS:
+ workload_key = self.active_workers.pop_by_id(job.job_id)
+ self.success(workload_key)
+ except Exception:
Review Comment:
Added the `ClientError` / `NoCredentialsError` re-raise you suggested in the
follow-up commit, so credential issues keep reaching `sync()`'s connection
handling even if code inside the block grows a boto call later.
On narrowing the catch further: the only concrete type we can name today is
`KeyError` from `pop_by_id()` on corrupted bookkeeping, but the failure class
this PR targets is "any deterministic error in per-job handling permanently
wedges the executor". Narrowing to the exceptions we've already met protects
only against the last incident — the next poison (a `TypeError` from a
malformed status, an `AttributeError` from a partial workload, …) would stall
submission cluster-wide again exactly as before.
Two things that hopefully make the broad catch less scary:
1. It doesn't widen what gets caught — before this PR the very same
exceptions were swallowed by `sync()`'s own `except Exception`; the change just
moves the boundary so the blast radius is one job instead of the whole
heartbeat.
2. On "will every possible exception here be constant?" — everything inside
the block is in-memory bookkeeping plus executor state-change calls, no I/O, so
in practice errors here are deterministic (that was our production case). If a
transient one did slip through, the cost is one task failed and retried per its
retry policy, versus today's cost of the whole sync cycle aborting. Happy to
add an eviction threshold (only evict after N consecutive per-job sync errors)
if you'd prefer to be more conservative.
---Drafted-by: Claude Code (Fable 5); reviewed by @AlexanderBLR before
posting
--
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]
