Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

We should strip the callstack printing from the timeout exception #517

Open
ianoc opened this issue Jun 9, 2014 · 1 comment
Open

We should strip the callstack printing from the timeout exception #517

ianoc opened this issue Jun 9, 2014 · 1 comment

Comments

@ianoc
Copy link
Collaborator

ianoc commented Jun 9, 2014

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)

@johnynek
Copy link
Collaborator

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants