refactor: extract shared job-status helper in DataLoad and DataMigra…#6067
Conversation
…te handlers OnceStatusHandler and OnEventStatusHandler in both DataLoad and DataMigrate contained near-identical logic for looking up the triggered job, checking its finished condition, and returning phase/conditions/duration. This duplicated four copies of the same ~40-line block. Extract a shared getJobOperationStatus() helper in each package so both handlers delegate to it. The helper takes a generateNodeAffinity bool so the DataMigrate parallelism guard (only set NodeAffinity when Parallelism==1) can be expressed cleanly at the call site. This was promised as a follow-up to PR fluid-cloudnative#5945 (review comment by cheyang). Closes fluid-cloudnative#6065 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @adityaupasani2. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the status handlers for both DataLoad and DataMigrate controllers by extracting common logic into a shared helper function, getJobOperationStatus. This helper handles retrieving the job, checking its completion status, and updating the operation status, thereby reducing code duplication in OnceStatusHandler and OnEventStatusHandler. As there are no review comments, I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6067 +/- ##
=======================================
Coverage 64.77% 64.77%
=======================================
Files 484 484
Lines 33892 33892
=======================================
Hits 21954 21954
Misses 10215 10215
Partials 1723 1723 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return getJobOperationStatus(ctx, r.Client, releaseName, jobName, ctx.Namespace, true, opStatus) | ||
| } | ||
| func (c *CronStatusHandler) GetOperationStatus(ctx cruntime.ReconcileRequestContext, opStatus *datav1alpha1.OperationStatus) (result *datav1alpha1.OperationStatus, err error) { | ||
| result = opStatus.DeepCopy() |
There was a problem hiding this comment.
Heads up on a small but real semantic shift here. In the old OnceStatusHandler.GetOperationStatus, when kubeclient.GetJob returned a NotFound error and helm.DeleteReleaseIfExists succeeded, control fell through the inner if and ended up at ctx.Log.Error(err, "can't get dataload job", ...) with err == nil, then returned. The new getJobOperationStatus helper adds an explicit return after the successful delete, so that spurious nil-error log line is no longer emitted for the Once path. This matches what the old OnEventStatusHandler already did and what DataMigrate already did, so the new behavior is the more reasonable one — but it is a behavior change and is worth calling out in the commit message so future readers don't have to dig for it.
| // It looks up the triggered job, checks its finished condition, and returns the updated | ||
| // OperationStatus with the correct phase, conditions and duration. If generateNodeAffinity | ||
| // is true and the job succeeded, it also populates NodeAffinity from the job's node labels. | ||
| func getJobOperationStatus(ctx cruntime.ReconcileRequestContext, c client.Client, releaseName, jobName, namespace string, generateNodeAffinity bool, opStatus *datav1alpha1.OperationStatus) (result *datav1alpha1.OperationStatus, err error) { |
There was a problem hiding this comment.
The PR title says "extract shared job-status helper in DataLoad and DataMigrate", but what actually landed is two package-local helpers — one getJobOperationStatus in pkg/controllers/v1alpha1/dataload and another getJobOperationStatus in pkg/controllers/v1alpha1/datamigrate. They are byte-identical apart from the log labels ("DataLoad" vs "DataMigrate" and the chart-existed info log). Within a single controller this PR removes ~80 lines of duplication, which is a clear win, but the cross-controller duplication that #6065 originally pointed at is still there. Not blocking — moving the helper to pkg/dataoperation (where the StatusHandler interface already lives) would require parameterizing the log labels and likely passing the logger explicitly, which is more invasive. Worth a follow-up issue if we want full dedup later.
| // It looks up the triggered job, checks its finished condition, and returns the updated | ||
| // OperationStatus with the correct phase, conditions and duration. If generateNodeAffinity | ||
| // is true and the job succeeded, it also populates NodeAffinity from the job's node labels. | ||
| func getJobOperationStatus(ctx cruntime.ReconcileRequestContext, c client.Client, releaseName, jobName, namespace string, generateNodeAffinity bool, opStatus *datav1alpha1.OperationStatus) (result *datav1alpha1.OperationStatus, err error) { |
There was a problem hiding this comment.
The existing OnceStatusHandler and OnEventStatusHandler test cases continue to pass and indirectly cover getJobOperationStatus. That's fine, and explicit coverage is not required for merge. If you want to firm things up, a small focused test on getJobOperationStatus itself — exercising the four branches (job missing + helm delete OK, job missing + helm delete fail, job running, job finished with generateNodeAffinity toggled) — would lock in the contract going forward and make any future regression obvious.
2 participants
Ⅰ. Describe what this PR does
OnceStatusHandlerandOnEventStatusHandlerin bothDataLoadandDataMigratecontained near-identical ~40-line blocks for looking up the triggered job, checking its finished condition, and returning phase/conditions/duration. This resulted in four copies of the same logic that had to be kept in sync.Extracts a shared
getJobOperationStatus()helper in each package so both handlers delegate to it. The helper takes agenerateNodeAffinity boolparameter so the DataMigrate parallelism guard (Parallelism == 1before setting NodeAffinity) can be expressed cleanly at the call site:Net result: 122 lines removed, future status-handling fixes are a single edit instead of four.
Ⅱ. Does this pull request fix one issue?
Closes #6065
Ⅲ. List the added test cases
No new tests needed — existing tests for all four handlers continue to pass unchanged.
Ⅳ. Describe how to verify it
go test ./pkg/controllers/v1alpha1/dataload/... ./pkg/controllers/v1alpha1/datamigrate/...Ⅴ. Special notes for reviews
This was promised as a follow-up to PR #5945 (review comment by cheyang requesting the refactor).