[#1209] improvement(server): Optimize cleanupStorageSelectionCache method in LocalStorageManager. - #1210
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1210 +/- ##
============================================
+ Coverage 53.68% 54.87% +1.19%
- Complexity 2588 2601 +13
============================================
Files 391 372 -19
Lines 22425 20126 -2299
Branches 1875 1887 +12
============================================
- Hits 12038 11044 -994
+ Misses 9682 8444 -1238
+ Partials 705 638 -67 ☔ View full report in Codecov by Sentry. |
zuston
left a comment
zuston
left a comment
There was a problem hiding this comment.
Nice catch @zhuyaogai .
Overall lgtm, left some minor comments.
| } | ||
| long startTime = System.currentTimeMillis(); | ||
| deleteElement(partitionsOfStorage, deleteConditionFunc); | ||
| deleteElement(partitionsOfStorage.tailMap(prefixKey), deleteConditionFunc); |
There was a problem hiding this comment.
If the perfixKey is null, it will throw NPE
| deleteConditionFunc = | ||
| partitionUnionKey -> UnionKey.startsWith(partitionUnionKey, event.getAppId()); | ||
| } else if (event instanceof ShufflePurgeEvent) { | ||
| assert event.getShuffleIds().size() == 1; // only one shuffleId |
There was a problem hiding this comment.
Could you log the size and ignore the other ids, rather than assert ? This is harmful for a running service
| event); | ||
| } | ||
|
|
||
| private <K, V> void deleteElement(Map<K, V> map, Function<K, Boolean> deleteConditionFunc) { |
There was a problem hiding this comment.
Could you help rename the map to sortedPartitionsOfStorageMap? If you don't mind, the comments could be added in this method. Because the deleting logic depends on the sorted map.
|
It is a bit weird that I can run successfully those failing unit tests locally... |
Maybe flaky tests. Do you mind submiting an issue to track and fix this ? If you want |
zuston
left a comment
zuston
left a comment
There was a problem hiding this comment.
lgtm. waiting for all tests passed
|
Thanks @zhuyaogai Merged/ |
|
@zuston Is this a bug fix? If so, we should commit to branch 0.8, too. |
I think yes. |
4 participants
What changes were proposed in this pull request?
Optimize cleanupStorageSelectionCache method in LocalStorageManager.
Why are the changes needed?
Fix: #1209
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Exist UTs.