DataprocCreateClusterOperator: Fix non-deferrable reconciliation and handle deletion during creation - #61951
Conversation
boring-cyborg
Bot
added
area:providers
provider:google
Yes. I will fix it. |
613a843 to
747bf37
Compare
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Thanks! I got a couple of comments.
To get this merged after applying the fixes - could you please run the system tests with your changes applied? If you're unable to do so please let me know and we'll figure it out.
de87984 to
d68d5d9
Compare
|
I have made a few changes in response to your comments. I have also ran all the system tests in:
All 24 tests passed. |
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Well done, LGTM!
@VladaZakharova @MaksYermak - what do you think?
|
Tagging you here for review. |
renu-j
left a comment
renu-j
left a comment
There was a problem hiding this comment.
Thanks for the changes! I had a couple of comments during the review.
One point is related to the use of assert_called_once_with() in the test, which appears to be used twice with different arguments. Since this assertion guarantees a single call, it may lead to inconsistent expectations if multiple calls are intended. It might be clearer to use assert_has_calls() if verifying multiple calls.
I also left a small question regarding the cluster state verification after recreating the cluster in the DELETING branch.
…iliation runs after creation completes. – Extract reconciliation logic into `_reconcile_cluster_state()` – Ensure DELETING state waits for deletion and re-creates the cluster – Ensure CREATING state is fully reconciled before returning – Handle STOPPED state via restart path – Raise explicit exception if cluster is not found after LRO completion – Return reconciled cluster to avoid stale state Update and extend unit tests to cover reconciliation scenarios in the non-deferrable path (CREATING, DELETING, STOPPED, ERROR, and timeout cases).
d68d5d9 to
130332c
Compare
|
I appreciate you taking out the time to review the PR. I have responded to your comments. Please review. |
…iliation runs after creation completes. (apache#61951) – Extract reconciliation logic into `_reconcile_cluster_state()` – Ensure DELETING state waits for deletion and re-creates the cluster – Ensure CREATING state is fully reconciled before returning – Handle STOPPED state via restart path – Raise explicit exception if cluster is not found after LRO completion – Return reconciled cluster to avoid stale state Update and extend unit tests to cover reconciliation scenarios in the non-deferrable path (CREATING, DELETING, STOPPED, ERROR, and timeout cases). Co-authored-by: Sameer Mesiah <smesiah971@gmail.com>
…iliation runs after creation completes. (apache#61951) – Extract reconciliation logic into `_reconcile_cluster_state()` – Ensure DELETING state waits for deletion and re-creates the cluster – Ensure CREATING state is fully reconciled before returning – Handle STOPPED state via restart path – Raise explicit exception if cluster is not found after LRO completion – Return reconciled cluster to avoid stale state Update and extend unit tests to cover reconciliation scenarios in the non-deferrable path (CREATING, DELETING, STOPPED, ERROR, and timeout cases). Co-authored-by: Sameer Mesiah <smesiah971@gmail.com>
4 participants
Description
This change refactors the
DataprocCreateClusterOperator.executemethod to ensure cluster state reconciliation is consistently applied in non-deferrable mode.After submitting the cluster creation request and waiting for the long-running operation (LRO) to complete, the operator now explicitly fetches the current cluster state and passes it through a dedicated
_reconcile_cluster_statemethod before returning success.The reconciliation logic, previously embedded inline in
execute, has been consolidated into_reconcile_cluster_state. This method handles clusters inCREATING,DELETING, andSTOPPEDstates by waiting, recreating, or restarting as appropriate.Rationale
The operator docstring specifies that when
use_if_exists=True, the operator should:CREATINGDELETINGERRORstate appropriatelyAlthough state-handling logic existed, the non-deferrable execution path previously returned immediately after the create LRO completed, preventing the existing reconciliation logic from being triggered in certain scenarios (e.g. cluster transitioning to
DELETINGduring creation).This change ensures the pre-existing reconciliation behavior is executed consistently, aligning runtime behavior with the documented contract.
Notes
NotFoundhandling after the Long-Running Operation (LRO) completes to surface a clearAirflowExceptionif the cluster was deleted before its state could be reconciled.Tests
Unit tests have been added to cover reconciliation scenarios:
AirflowExceptionwhen the cluster remains inDELETINGstate and deletion is not triggered.delete_on_error=True.Existing tests have been updated to align with the new reconciliation flow and state handling behavior.
Backwards Compatibility
There is no intended change to the operator’s public contract. The implementation now consistently executes the previously defined reconciliation logic in non-deferrable mode.
Closes: #61947