ISPN-4677 Map/Reduce job fails with the wrong explanation on TimeoutException #2844
Conversation
| @@ -671,8 +671,7 @@ protected void executeMapPhaseWithLocalReduction(Map<KOut, VOut> reducedResult) | |||
| Throwable cause = ee.getCause(); | |||
| if (cause instanceof org.infinispan.util.concurrent.TimeoutException) { | |||
| throw new ExecutionException("Map phase executing at " + mapTaskPart.getAddress() | |||
| + " did not complete within " + rpcOptionsBuilder.timeout(TimeUnit.SECONDS) + " sec timeout", | |||
| cause); | |||
| + " did not complete", cause); | |||
| } else { | |||
| throw ee; | |||
There was a problem hiding this comment.
Looking at this closer, is there a reason we just don't throw the underlying cause? That is usually more customary when rethrowing an ExecutionException.
There was a problem hiding this comment.
Because Sanne suggested that we be more descriptive about the exception cause. It however turned out that we have more than one reason why this call may timeout with TimeoutException. So it is incorrect to claim that the "call did not complete within " certain timeout. That is why the wording is a bit more generic. I really think we should not spend more time on this "trivial" issue :-)
There was a problem hiding this comment.
Throwing the outer ExecutionException doesn't provide any additional information though. It just exposes underlying implementation details showing that we most likely use an ExecutorService is all. Think what I was trying to say might have got lost in translation.
There was a problem hiding this comment.
Aha, I get you! Yeah, it makes sense to peel off ExecutionException I think. No reason for it to be there if after all we throw CacheException to the client. Let me experiment and see what the effect is!
There was a problem hiding this comment.
Actually I think this is ok @wburns We want to provide additional information about Map phase executing at certain node not completing.
|
@Sanne @wburns @danberindei let me know if this makes more sense now. |
| future.get(); | ||
| try { | ||
| future.get(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Thinking if we got an ExecutionException we should unwrap that and put the cause in the MapReduceException instead so we don't have another cause by in stack that really provides no benefit.
There was a problem hiding this comment.
Also looking at this closer, I don't like the idea that we are just rewrapping the InterrruptedException. It may be the best thing but normally I like to propagate those to the caller if it is possible to respond in a better fashion. Maybe that isn't possible here.
There was a problem hiding this comment.
Yep, you are right! I'll unwrap ExecutionException and propagate InterruptedException. Thanks @wburns
…ng TimeoutException
|
Rebased and ready for furher review. @wburns @danberindei could you please have a look |
danberindei
force-pushed
the
master
branch
3 times, most recently
from
8465ee1
to
8c3c08c
Compare
|
Integrated, thanks Vladimir! |
|
great, thanks!! |
Master only. See https://issues.jboss.org/browse/ISPN-4677 for discussion and rationale.