Decouple SparkSubmitOperator resumable deployment backends - #68679
Conversation
|
@SameerMesiah97 could you review the new PR please. |
Okay. But for future reference, it is better to revise the existing PR as previous feedback is easier to trace. |
SameerMesiah97
left a comment
There was a problem hiding this comment.
The use of private classes fixed my main concern. And looking at your comment in the closed PR #68543, it seems like it is plausible that additional backends may be added in the near future so the class-based approach is defensible.
This is basically a lift and shift of much of the existing operator logic and into these classes. So I checked if any bugs were introduced by accident and could not find any. There is a CI failure so I would fix that. No further reservations.
|
@onlyarnav — the Static checks CI job is failing here, which needs a code fix on your side (a rerun won't clear it). You can reproduce and fix locally with: Once those pass and CI is green, it'll be ready for a maintainer to pick up. Thanks! See the PR quality criteria. Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
df35fda to
b411423
Compare
|
@potiuk Thanks for pointing that out! |
|
Hi @jason810496 , I've put together a possible approach for decoupling the |
potiuk
left a comment
There was a problem hiding this comment.
Thanks — this reads as a clean behaviour-preserving refactor: the K8s and YARN branches move out of the inline conditionals into explicit backend classes without changing what they do, and the existing suite in test_spark_submit.py already covers these paths (app-id persistence, retry/reconnect, wait_app_completion, on_kill, K8s submit and missing-pod-name), so the green run is meaningful here.
One nit while you're touching the line — a pre-existing string-concat bug comes along for the ride in _YarnSparkSubmitBackend.submit_job:
"spark.yarn.submit.waitAppCompletion=true cannot be set for cluster mode as it conflicts"
"with the need to exit spark-submit immediately to persist the application ID for tracking. "renders as "…as it conflictswith the need…". Worth adding the trailing space in a follow-up.
Optional follow-up thought, not for this PR: now that the backends are a formal interface, the amount of hook._conf / hook._kubernetes_driver_pod / hook._poll_k8s_driver_via_api reaching-through is more visible. Promoting a few of those to public hook methods would make the interface stand on its own.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Description
The
ResumableJobMixinimplementation forSparkSubmitOperatorpreviously had YARN, Kubernetes, and Standalone backend logics interleaved directly inside each mixin method. This scattered per-backend logic across multiple methods. Decoupling these by introducing specialized strategy classes per backend isolates the deployment-specific details, making the operator easier to maintain and extend.This refactor:
_SparkSubmitDeploymentBackend,_KubernetesSparkSubmitBackend,_YarnSparkSubmitBackend,_StandaloneSparkSubmitBackend) to cleanly isolate the status and execution tracking logic of each deployment mode._backendcached property onSparkSubmitOperator.submit_job,get_job_status,is_job_active,is_job_succeeded,poll_until_complete,on_kill) directly to the active backend strategy.This improves maintainability while keeping public namespaces completely clean.
fixes #68505
Was generative AI tooling used to co-author this PR?