-
Notifications
You must be signed in to change notification settings - Fork 134
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
[ISSUE-484] Fix accidentally remove the storage of appId when unregistering partial shuffle in HdfsStorageManager #485
Conversation
…tering partial shuffle in HdfsStorageManager
PTAL @jerqi . I think this is an important bug fix. |
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
============================================
- Coverage 58.77% 58.76% -0.02%
- Complexity 1704 1705 +1
============================================
Files 206 206
Lines 11468 11470 +2
Branches 1024 1023 -1
============================================
Hits 6740 6740
- Misses 4317 4319 +2
Partials 411 411
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
Outdated
Show resolved
Hide resolved
Looks like there's some flaky test? Overall, this LGTM. |
This flaky test has been recorded in #483. Let's rerun it. |
PTAL @advancedxy CI passed. |
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.
overall, LGTM.
If @jerqi has time, you can have another look.
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
Merged. Thanks for your review @jerqi @advancedxy |
What changes were proposed in this pull request?
When one app's partial shuffles are removed, it will make other shuffle fail of flushing data due to its missing storage referenced from its appId.
Why are the changes needed?
Fix accidentally remove the storage of appId when unregistering partial shuffle in HdfsStorageManager
Does this PR introduce any user-facing change?
NO
How was this patch tested?