refactor(alluxio): remove redundant CacheStates update from UpdateDat…#5806
Conversation
…asetStatus CacheStates is already managed by UpdateCacheOfDataset() at Sync step 5. The duplicate assignment in UpdateDatasetStatus() has no effect and adds an unnecessary getRuntime() call. Signed-off-by: lin121291 <4jp33f9e@gmail.com>
|
Hi @lin121291. 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. |
|
There was a problem hiding this comment.
Code Review
This pull request simplifies the UpdateDatasetStatus function in the Alluxio engine by removing the retrieval of runtime status and the redundant update of CacheStates. These changes streamline the status update process, as CacheStates are managed in a separate function. I have no feedback to provide as no review comments were present.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5806 +/- ##
==========================================
- Coverage 58.17% 58.17% -0.01%
==========================================
Files 478 478
Lines 32485 32477 -8
==========================================
- Hits 18899 18894 -5
+ Misses 12042 12040 -2
+ Partials 1544 1543 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Alluxio runtime’s dataset status update flow by removing a redundant CacheStates write and an unnecessary getRuntime() fetch from AlluxioEngine.UpdateDatasetStatus(), keeping cache state updates centralized in UpdateCacheOfDataset() (called from the base sync flow).
Changes:
- Removed
getRuntime()call fromAlluxioEngine.UpdateDatasetStatus(). - Removed redundant assignment of
dataset.Status.CacheStatesinsideUpdateDatasetStatus()(avoids duplicatingUpdateCacheOfDataset()behavior). - Dropped the related TODO/commented-out lines that indicated the duplication.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The change is correct. The
Removing the redundant Verified against the sync flow in LGTM. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |



Ⅰ. Describe what this PR does
Remove redundant
CacheStatesassignment and unnecessarygetRuntime()call from
AlluxioEngine.UpdateDatasetStatus().UpdateDatasetStatus()manages dataset phase and conditions. It alsosets
CacheStates, duplicating the work ofUpdateCacheOfDataset()which is the authoritative source (called at Sync step 5 in
pkg/ddc/base/syncs.go:79).The assignment is a no-op in all code paths:
UpdateCacheOfDataset()overwrites immediately afterCheckAndUpdateRuntimeStatusnever ran,so
runtime.Status.CacheStatesalready equalsdataset.Status.CacheStatesfrom last successful sync
The existing TODO comment in the code confirms this was a known concern.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases
No new tests needed. The removed code was a no-op — existing tests for
UpdateDatasetStatus()andUpdateCacheOfDataset()continue to pass.Ⅳ. Describe how to verify it
FLUID_UNIT_TEST=true go test ./pkg/ddc/alluxio/... -count=1notes