zuston commented on code in PR #424:
URL: https://github.com/apache/incubator-uniffle/pull/424#discussion_r1050705734
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -139,32 +140,55 @@ public class LocalStorageManager extends
SingleStorageManager {
@Override
public Storage selectStorage(ShuffleDataFlushEvent event) {
- LocalStorage storage =
localStorages.get(ShuffleStorageUtils.getStorageIndex(
- localStorages.size(),
- event.getAppId(),
- event.getShuffleId(),
- event.getStartPartition()));
- if (storage.containsWriteHandler(event.getAppId(), event.getShuffleId(),
event.getStartPartition())
- && storage.isCorrupted()) {
- LOG.error("storage " + storage.getBasePath() + " is corrupted");
- }
- if (storage.isCorrupted()) {
- storage = getRepairedStorage(event.getAppId(), event.getShuffleId(),
event.getStartPartition());
+ String appId = event.getAppId();
+ int shuffleId = event.getShuffleId();
+ int partitionId = event.getStartPartition();
+
+ try {
+ LocalStorage storage =
partitionsOfStorage.get(appId).get(shuffleId).get(partitionId);
+ if (storage.isCorrupted()) {
+ throw new RuntimeException("LocalStorage: " + storage.getBasePath() +
" is corrupted.");
Review Comment:
Sorry, I missed this thread.
> The whole partition could not be trusted. ShuffleReadClient should switch
to another replica as soon as possible.
Maybe there's some logic I didn't follow?
Let me explain more detail about this. Firstly it need to be corrected that
the disk corruption will influence the writing process in current case. Of
course, it will cause the later reading if having multiple replica.
So if having single replica and using the `MEMORY_LOCALFILE` or `LOCALFILE`
type, once disk is corrupted, which means the data lost, we should make
relevant job fast fail. But in current codebase, it ignore this failure and
retry and then fail. Of course, this will not cause big problems.
But when using `MEMORY_LOCALFILE_HDFS` and single replica, this will cause
rest events flushing to fallback storage, like from localdisk to HDFS.
Actually, this is unnecessary, as partial data lost.
Of course, this could be useful for multiple replica. But I think we could
support this mechanism after dynamic selection, which will be better.
> ShuffleReadClient should switch to another replica as soon as possible.
Yes. I think this is reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
