abstractdog commented on code in PR #150:
URL: https://github.com/apache/tez/pull/150#discussion_r1118058491
##########
tez-runtime-library/src/main/java/org/apache/tez/runtime/library/common/shuffle/orderedgrouped/ShuffleScheduler.java:
##########
@@ -1015,51 +1019,63 @@ boolean isShuffleHealthy(InputAttemptIdentifier
srcAttempt) {
final float MIN_REQUIRED_PROGRESS_PERCENT = minReqProgressFraction;
final float MAX_ALLOWED_STALL_TIME_PERCENT = maxStallTimeFraction;
- int doneMaps = numInputs - remainingMaps.get();
+ String errorMsg = null;
Review Comment:
I think instead of creating synchronized block here, it's time to refactor
this awful piece of code...we're reporting exceptions and returning with a
vague boolean, which is not okay, instead, we should do it in copyFailed: catch
all IOExceptions and report them, what about:
```
//Restart consumer in case shuffle is not healthy
try{
checkIfShuffleIsHealthy(srcAttempt)
} catch(IOExcepion e){
exceptionReporter.reportException(ioe);
return;
}
```
in checkIfShuffleIsHealthy we can do separate things synchronized and don't
have to return boolean:
```
void checkIfShuffleIsHealthy(){
checkIfAbortLimitIsExceeedFor(srcAttempt)
checkWhateverTheRestOfTheMethodDoes(srcAttempt)
}
```
regarding checkWhateverTheRestOfTheMethodDoes:
1. this is the logic you're about to make synchronized, you can make it I
guess (as exception reporting is handled in caller method copyFailed as
suggested above
2. find a proper method name for checkWhateverTheRestOfTheMethodDoes, which
reflects what it actually does
--
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]
