[Improvement] Ignore partial failure on initializing local storage in shuffle server side #102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
============================================
+ Coverage 56.45% 56.55% +0.09%
- Complexity 1182 1183 +1
============================================
Files 149 149
Lines 8008 8019 +11
Branches 767 767
============================================
+ Hits 4521 4535 +14
+ Misses 3243 3241 -2
+ Partials 244 243 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. |
| assertEquals(2, localStorageManager.getStorages().size()); | ||
| } | ||
|
|
||
| public static class MockedLocalStorageBuilder extends LocalStorage.Builder { |
There was a problem hiding this comment.
Could we use a path which don't exist instead of MockedLocalStorageBuilder? Such as we use a path named /xxx/yyy, but /xxx don't exist.
There was a problem hiding this comment.
Got it.
But i can't understand why this mockBuilder is not appropriate.
There was a problem hiding this comment.
Got it. But i can't understand why this mockBuilder is not appropriate.
The Builder some fields and method become protected. I feel it's not necessary.
There was a problem hiding this comment.
If the public modifier is not appropriate, i could change to protected. And provide a method of getBasePath().
What do u think so?
| LOGGER.error("Initializing the storage path of {} failed.", storagePath, exception); | ||
| } | ||
| } | ||
| if (localStorages.size() <= 0) { |
There was a problem hiding this comment.
Should we introduce a configOption that if the broken disks are more than the value, the start shuffle will fail to start.
…ng multiple local storage paths in shuffle server client
| } | ||
|
|
||
| localStorages = Arrays.asList(localStorageArray); | ||
| localStorages = Arrays.stream(localStorageArray).filter(Objects::nonNull).collect(Collectors.toList()); |
There was a problem hiding this comment.
The localStorageArray maybe exist the null in some index, this will make the localStorages exist the null element when using the Arrays.asList(localStorageArray)
There was a problem hiding this comment.
Oh, my fault. Ignore this
| } | ||
|
|
||
| localStorages = Arrays.asList(localStorageArray); | ||
| localStorages = Arrays.stream(localStorageArray).filter(Objects::nonNull).collect(Collectors.toList()); |
| // ignore | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you add a new line? Because there are a warning if we don't have a newline at the end of the file
3 participants
What changes were proposed in this pull request?
Ignore partial failure on initializing local storage in shuffle server side
Why are the changes needed?
When setting multiple storage paths, only one disk is insufficient capacity, shuffle server will directly throw exception. We hope it can be ignored, because bad disks are more common in production environments
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT