refactor: deprecate worker daemonset handling logic#5352
Conversation
- Remove deprecated mode checks and related code from various components - Delete IsDeprecated function from errors package - Update worker setup and status check functions to handle errors consistently Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Summary of ChangesHello @TrafalgarZZZ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Fluid project by eliminating the deprecated worker DaemonSet handling logic. Since worker DaemonSets were deprecated in Fluid v0.7.0, this change removes outdated compatibility checks and error handling mechanisms across the codebase, resulting in a more streamlined and maintainable system. The focus is on simplifying the code by removing components that are no longer supported or necessary, thereby reducing complexity and potential for future issues related to legacy code paths. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 effectively removes the deprecated worker daemonset handling logic, which simplifies the codebase significantly. The changes are consistent across all affected components, and the removal of fluiderrs.IsDeprecated checks cleans up the error handling paths.
Notably, this refactoring also fixed a potential nil pointer dereference bug in pkg/ddc/goosefs/status.go where an error from GetWorkersAsStatefulset was not being properly handled, which could lead to a panic.
I have one suggestion regarding potentially unused code that was modified as part of this refactoring. Other than that, the changes look good.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively removes the deprecated logic for handling worker daemonsets, which simplifies the codebase and improves maintainability. The changes are mostly about removing the now-unused IsDeprecated error checks and associated code. My review found a couple of minor issues: an outdated comment and an unused function that should also be removed for consistency. Overall, this is a good cleanup effort.
| // GetWorkersAsStatefulset gets workers as statefulset object. if it returns deprecated errors, it indicates that | ||
| // not support anymore. |
There was a problem hiding this comment.
The function comment is now outdated as the logic for handling deprecated DaemonSet workers has been removed. The function no longer returns deprecated errors. The comment should be updated to reflect its current functionality.
| // GetWorkersAsStatefulset gets workers as statefulset object. if it returns deprecated errors, it indicates that | |
| // not support anymore. | |
| // GetWorkersAsStatefulset gets workers as a statefulset object. |
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5352 +/- ##
==========================================
- Coverage 56.96% 56.85% -0.12%
==========================================
Files 442 442
Lines 30529 30461 -68
==========================================
- Hits 17391 17318 -73
- Misses 11522 11527 +5
Partials 1616 1616 ☔ 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 removes deprecated worker daemonset handling logic that was deprecated since Fluid v0.7.0. The changes simplify the codebase by removing backward compatibility code that is no longer needed.
- Removes
IsDeprecatederror checking throughout the codebase - Deletes deprecated mode test cases across all runtime engines
- Simplifies error handling by removing special cases for deprecated daemonset workers
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/errors/deprecated.go | Comments out the IsDeprecated function |
| pkg/errors/errors_test.go | Removes tests for the deprecated error checking |
| pkg/ctrl/worker.go | Simplifies GetWorkersAsStatefulset to only retrieve StatefulSet workers |
| pkg/ddc/*/worker.go | Removes deprecated error checks from worker setup across all engines |
| pkg/ddc/*/status.go | Removes deprecated error checks from status sync across all engines |
| pkg/ddc/*/replicas.go | Removes deprecated error checks from replica sync across all engines |
| pkg/ddc/*/sync_runtime.go | Removes deprecated error checks from runtime sync (juicefs, jindofsx, jindocache) |
| pkg/ddc//_test.go | Removes deprecated test cases and test data across all engines |
| pkg/ddc/base/log_test.go | Updates test to use conflict error instead of deprecated error |
| pkg/utils/dataset/lifecycle/node.go | Removes deprecated error check from node lifecycle handling |
Comments suppressed due to low confidence (3)
pkg/ddc/vineyard/replicas_test.go:1
- Using
breakinstead ofcontinuewill exit the entire test loop when the deprecated field is true. Since the deprecated field and test case have been removed, this code is now unreachable but should have usedcontinueto skip only that iteration, not break the entire loop. However, since the deprecated test case is removed, this code block should also be removed entirely.
pkg/ddc/goosefs/replicas_test.go:1 - Using
breakinstead ofcontinuewill exit the entire test loop when the deprecated field is true. Since the deprecated field and test case have been removed, this code is now unreachable but should have usedcontinueto skip only that iteration, not break the entire loop. However, since the deprecated test case is removed, this code block should also be removed entirely.
/*
pkg/ddc/jindocache/replicas_test.go:1
- This code block checks for a
deprecatedfield that no longer exists in the test case struct (removed on line 278). This will cause a compilation error. The entire if block should be removed since there are no more deprecated test cases.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // func IsDeprecated(err error) (deprecated bool) { | ||
| // return ReasonForError(err) == StatusReasonDeprecated | ||
| // } |
There was a problem hiding this comment.
The IsDeprecated function should be completely removed rather than commented out. Since the PR is removing deprecated worker daemonset handling logic, and this function is no longer called anywhere in the codebase, leaving it as commented code reduces maintainability. Consider removing it entirely or documenting why it needs to be preserved as comments.
| e.Log.Info("Warning: Deprecated mode is not support, so skip handling", "details", err) | ||
| return ready, nil | ||
| } | ||
| return ready, err |
There was a problem hiding this comment.
[nitpick] The error return statement is now missing a blank line before the closing brace on line 52. While this is a minor style issue, maintaining consistent spacing improves code readability. Consider adding a blank line after line 51 to match the pattern used elsewhere in the codebase.
| return ready, err | |
| return ready, err |
|
[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
Runtime worker daemonset has been deprecated since Fluid v0.7.0. It has been long enough to safely remove the logic for backward compatibility. This PR removes it to simplify code
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews