summingbird icon indicating copy to clipboard operation
summingbird copied to clipboard

We should strip the callstack printing from the timeout exception

Open ianoc opened this issue 11 years ago • 1 comments

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)

ianoc avatar Jun 09 '14 17:06 ianoc

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.

johnynek avatar Jun 30 '14 21:06 johnynek