Skip to content

Refactor SparkSubmitOperator resumable job tracking backends - #68543

Closed
onlyarnav wants to merge 4 commits into
apache:mainfrom
onlyarnav:refactor-spark-submit-resumable-backends
Closed

Refactor SparkSubmitOperator resumable job tracking backends#68543
onlyarnav wants to merge 4 commits into
apache:mainfrom
onlyarnav:refactor-spark-submit-resumable-backends

Conversation

@onlyarnav

Copy link
Copy Markdown
Contributor

Decouples the three resumable deployment backends (Spark standalone driver-status tracking, YARN cluster mode, and Kubernetes driver-pod tracking) in SparkSubmitOperator.

Problem

Previously, each method in the ResumableJobMixin implementation of SparkSubmitOperator (submit_job, get_job_status, is_job_active, is_job_succeeded, poll_until_complete, on_kill) branched inline on the active deployment backend. This spread backend-specific logic across multiple methods, making the codebase hard to follow and difficult to extend.

Solution

  1. Introduced a Strategy pattern:
    • SparkSubmitResumableBackend acts as the abstract base class/interface.
    • YarnSparkSubmitBackend, KubernetesSparkSubmitBackend, and StandaloneSparkSubmitBackend encapsulate backend-specific logic.
  2. Added a cached _resumable_backend property in SparkSubmitOperator to resolve the backend selection exactly once (lazily on first access).
  3. Delegated all the mixin methods in SparkSubmitOperator directly to the active strategy backend, keeping the operator clean.

closes: #68505


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.8)

@onlyarnav

Copy link
Copy Markdown
Contributor Author

There were some test failing so I made a new commit in the branch from my own forked repo. Hope all tests should pass now.

@onlyarnav

Copy link
Copy Markdown
Contributor Author
  1. assert hook is not None added after every hook = self.operator._hook across all four backend classes (and inline calls like self.operator._hook.on_kill() were split into two lines first).

  2. self._cached_resumable_backend: SparkSubmitResumableBackend | None = None added in __init__.

  3. _resumable_backend propertyhasattr replaced with if self._cached_resumable_backend is not None, and backend: SparkSubmitResumableBackend declared before the if/elif/else block.

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few issues with this PR:

  1. You are adding 4 new public classes with public methods when the intention appears to be improving maintainability rather than exposing new functionality to end users.

  2. I see that a lot of these public methods are effectively relatively thin wrappers around existing hook methods. The additional logic is also largely displaced lifecycle logic from the operator.

  3. The original issue hinted at a consolidated group of private methods instead of these new classes you are introducing. Arguably, what you are doing right now is better if we anticipate that new backends will be introduced in the near future, but how probable is that at the moment? If we do not expect new backends, would private helper methods be the better solution?

@jason810496 might disagree with me though and I would follow his lead if that is the case. But these are just my thoughts.

@onlyarnav

Copy link
Copy Markdown
Contributor Author

1

Sorry for the public exposure, that wasn't intentional, will prefix all four classes with an underscore so they are private.

2.1

You're right that the the post-submit-commands handling in poll_until_complete ; finally block wasn't really backend specific, it was operator cleanup that happened to be duplicated across YARN and standalone. i'll pull it into a shared helper on the base class: each backend will just passes its tracking call in as a closure

2.2

Thin wrapper observation by Claude Opus 4.8:

Methods like get_job_status being a near-direct call to hook.query_yarn_application_status() is by design rather than an oversight: each backend method exists specifically to isolate the one call that varies per deployment mode, so a thin method at that seam is the abstraction working correctly, not leftover bloat. If we collapsed those down further we'd just be re-introducing the branching this refactor was meant to remove.

3

On the class vs private methods point: I would lean toward keeping the class based approach. Looking at the issue background, standalone or YARN or K8s tracking were added in separate PRs over time, which suggests this is an area that keeps growing. With the strategy classes, adding a future backend means writing one new class against a fixed interface with no risk to existing backends or their tests otherwise with grouped private methods we will be back to editing six dispatch points per addition (basically the original problem).

Let me the make the new commits asap

@onlyarnav

Copy link
Copy Markdown
Contributor Author

i should make a new pr for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor SparkSubmitOperator resumable backends into separate methods/classes

2 participants