-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#794] feat(operator): support delete ShuffleServer pod with Evicted status #795
Conversation
Codecov Report
@@ Coverage Diff @@
## master #795 +/- ##
============================================
- Coverage 60.67% 58.08% -2.60%
- Complexity 1901 2072 +171
============================================
Files 239 304 +65
Lines 13031 14816 +1785
Branches 1091 1212 +121
============================================
+ Hits 7907 8606 +699
- Misses 4686 5720 +1034
- Partials 438 490 +52
... and 109 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cloud you add an ut for this change? The related ut should already been there |
I have tried but i found it is difficult. Because this operation involves updating the exclude-nodes and checking whether the shuffle server pod can be deleted. I think we need test it by integration test. |
I believe you may extract the status check into a small method and add ut for that small method. You can check how rss_test.go is constructed. |
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, exception one minor comment.
BTW, the ci is failed, you may have to look into it.
What changes were proposed in this pull request?
Support delete ShuffleServer pod with Evicted status
Why are the changes needed?
When ShuffleServer pod is Evicted, we can't delete it.
Fix: #794
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Manual testing