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 strange and confusing 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(e);
return;
}
```
in checkIfShuffleIsHealthy we can do separate things in a synchronized way
and don't have to return a 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
with the method refactoring, you don't have to introduce huge synchronized
block, instead you can make it clear with a convenient method name what is it
to be syncronized
does it make sense @glapark ?
--
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]
