johnynek
commented
10 years ago So, if a the future fails, we handle it here: https://github.com/twitter/summingbird/blob/develop/summingbird-online/src/main/scala/com/twitter/summingbird/online/executor/AsyncBase.scala#L45
The issue in on line 95 is that when we get an exception, we are not differentiating between a failure or the future, or a timeout. We could liftToTry, and then the only way we get an exception is if we timeout. But even then, we have to be careful to not not create a race: what if the future completes just after that time? It may actually be correct, and just really subtle. We should add some comments and directly test AsyncBase.
We need to clean that code up and make sure all the paths are correct. I agree.
In fact, I wonder if this is creating some of the noisy storm tests.
https://github.com/twitter/summingbird/blob/f64ec6b8de54242c6be712502d315d9dd99e678a/summingbird-online/src/main/scala/com/twitter/summingbird/online/executor/AsyncBase.scala#L95
It doesn't add anything to expose an internal callstack there.
Also:
We should be bubbling this up to the reportError function of the base bolt -- right now this won't appear in the nimbus UI and requires examining logs. (We also fail no tuples, not sure we can remedy this in these code paths, but if we use a raiseWithin on futures generated elsewhere we could attach the Tuple's to the Exception)