[SPARK-52581]Revise AppCleanUpStep to include cleanup logic for all states #257
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tates ### What changes were proposed in this pull request? This PR updates AppCleanUpStep so that it can be added as a regular step of each reconciliation, remove the therefore-unused AppTerminatedStep ### Why are the changes needed? Previously, we have AppTerminatedStep performed at the beginning of each reconciliation, to end the actions early if app has reached terminated state. However, in some scenarios, we may still want to do "clean up" for applications that are terminated: * If an app in "terminated without releasing resources" is being deleted, we would like the AppCleanUpStep to be performed, since it covers a few corner cases that might not be covered in general delete flow * We may also to introduce the concept of "maximal retention duration" for applications along with current "retention policy" in near future to serve the lifecycle management of application secondary resources. That could also require cleaning up secondary resources for already-terminated applications. The revised AppCleanUpStep includes everything in the previous AppTerminatedStep and takes care of cleaning up resources for those in "TerminatedWithoutReleaseResources" stage as needed. ### Does this PR introduce any user-facing change? No - internal step(s) changes only ### How was this patch tested? CIs and E2Es shall cover the clean up logic ### Was this patch authored or co-authored using generative AI tooling? No
jiangzho
force-pushed
the
cleanupForCancel
branch
from
420389c to
07b4f68
Compare
dongjoon-hyun
approved these changes
Jun 28, 2025
Member
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Sorry for the delay. I used sick days for Thursday and Friday.
+1, LGTM. Thank you, @jiangzho . I'll merge this and test more.
Member
|
BTW, I'm preparing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR updates AppCleanUpStep so that it can be added as a regular step of each reconciliation, remove the therefore-unused AppTerminatedStep
Why are the changes needed?
Previously, we have AppTerminatedStep performed at the beginning of each reconciliation, to end the actions early if app has reached terminated state.
However, in some scenarios, we may still want to do "clean up" for applications that are terminated:
The revised AppCleanUpStep includes everything in the previous AppTerminatedStep and takes care of cleaning up resources for those in "TerminatedWithoutReleaseResources" stage as needed.
Does this PR introduce any user-facing change?
No - internal step(s) changes only
How was this patch tested?
CIs and E2Es shall cover the clean up logic
Was this patch authored or co-authored using generative AI tooling?
No