[#1111] fix: Shuffle server can not delete remote storage path in security cluster #1122
Conversation
…in security cluster.
|
@zuston Could you help me review this pr? |
Codecov Report
@@ Coverage Diff @@
## master #1122 +/- ##
============================================
- Coverage 56.64% 54.56% -2.09%
- Complexity 2087 2413 +326
============================================
Files 317 351 +34
Lines 13782 18463 +4681
Branches 1228 1734 +506
============================================
+ Hits 7807 10074 +2267
- Misses 5559 7793 +2234
- Partials 416 596 +180
... and 75 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
common/src/test/java/org/apache/uniffle/common/KerberizedHadoop.java
Outdated
Show resolved
Hide resolved
jerqi
changed the title
| @@ -649,6 +649,7 @@ public void checkLeakShuffleData() { | |||
| public void removeResources(String appId) { | |||
| LOG.info("Start remove resource for appId[" + appId + "]"); | |||
| final long start = System.currentTimeMillis(); | |||
| String user = getUserByAppId(appId); | |||
There was a problem hiding this comment.
user may be null some times If app is removed.
There was a problem hiding this comment.
usermay be null some times If app is removed.
@jerqi
Only next line will remove app from shuffleTaskInfo.
Here I move getUserByAppId before 'shuffleTaskInfos.remove(appId)', or user will be null.
In fact the user in AppPurgeEvent is always null, but is not problem in non security cluster.
There was a problem hiding this comment.
If the method can handle the null value situation, I'm ok for this change. Let @zuston take another look.
|
@zuston Ping. |
What changes were proposed in this pull request?
Fix the bug that shuffle server can not delete remote storage path in security hdfs.
Why are the changes needed?
Fix: #1111
How was this patch tested?
unit test and test in cluster.