Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move cleanup into the executing thread (#1214) #1218

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jan 7, 2019

Fixes #1214

Before this PR, dbt directly raised an exception on failures inside the thread. But dbt also uses the thread pool's callback argument to handle marking the node as completed and put new nodes into the queue. That was great as long as the call in the thread always succeeded, but on exception the callback isn't called so the queue never gets finished and dbt hangs forever, resulting in the linked issue.

To fix this, instead the thread will mark an error message.

In the case of multiple concurrent error messages and some precise thread scheduling behavior, we will overwrite one or the other. This isn't really any different from before.

Threads that raise exceptions bypass the callback, which makes dbt hang.
Now threads don't raise during the callback, instead they set a flag.
The RunManager will check the flag during queue processing and raise if set.
Fix compilation failures so they raise properly.
@beckjake beckjake changed the title remove the callback and move cleanup into the executing thread (#1214) move cleanup into the executing thread (#1214) Jan 7, 2019
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great, LGTM when the tests pass

@beckjake beckjake merged commit 7179d13 into dev/grace-kelly Jan 7, 2019
@beckjake beckjake deleted the fix/lets-not-hang-on-errors branch January 7, 2019 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants