[Improvement] Ignore partial failure on initializing local storage in shuffle server side#102
Conversation
| 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?
There was a problem hiding this comment.
Updated. Remove this class @jerqi
| 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()); |
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