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

Fix adapter reset race condition in lib.py #5921

Merged
merged 3 commits into from
Sep 26, 2022
Merged

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Sep 23, 2022

resolves #5919

Description

This PR updates the lib.py file to avoid mutating global adapter state when generating a RuntimeConfig. This logic was once necessary in order to work around issues with dbt deps, though it's fairly clear at this point that we'll need to employ a radically different approach for deps in the future (including higher-order, first-class APIs). In the meantime, this change fixes possible race conditions for concurrent callers of get_dbt_config

Additional changes:

  • Change RuntimeArgs from a namedtuple to a dataclass fix an error in dbt v1.3.x (via this PR, i think)
  • Tweaked / fixed up code comments :)
  • Made it possible to control manifest sql operation nodes names. Historically, they've been hard-coded to "name". The default value is query for backwards compatibility. Just added an optional arg for this

TODO:

  • write tests (would love guidance on the best place to test this! Not sure that we want/need to repro the race condition, but a couple of tests for lib.py would be dope here)
  • figure out how to run changie again
  • almost positive black and flake8 are going to yell at me

Checklist

@drewbanin drewbanin requested review from a team and iknox-fa September 23, 2022 18:09
@cla-bot cla-bot bot added the cla:yes label Sep 23, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@drewbanin drewbanin requested a review from a team as a code owner September 23, 2022 21:45
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

I agree that at some point it would be good to have tests for lib.py. I'm not exactly sure what they'd look like though. I'm okay with doing that as a separate effort.

@drewbanin drewbanin merged commit 4e8aa00 into main Sep 26, 2022
@drewbanin drewbanin deleted the fix/lib-no-reset-adapter branch September 26, 2022 14:26
@leahwicz leahwicz added the backport 1.2.latest This PR will be backported to the 1.2.latest branch label Sep 26, 2022
github-actions bot pushed a commit that referenced this pull request Sep 26, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)
@leahwicz leahwicz added backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch backport 1.1.latest labels Sep 26, 2022
@github-actions
Copy link
Contributor

The backport to 1.0.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.0.latest 1.0.latest
# Navigate to the new working tree
cd .worktrees/backport-1.0.latest
# Create a new branch
git switch --create backport-5921-to-1.0.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4e8aa007cf17fff7605df1bb38eda2842dc9ba50
# Push it to GitHub
git push --set-upstream origin backport-5921-to-1.0.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.0.latest

Then, create a pull request where the base branch is 1.0.latest and the compare/head branch is backport-5921-to-1.0.latest.

github-actions bot pushed a commit that referenced this pull request Sep 26, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)
leahwicz pushed a commit that referenced this pull request Sep 27, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)

Co-authored-by: Drew Banin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch backport 1.1.latest backport 1.2.latest This PR will be backported to the 1.2.latest branch cla:yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1233] [Bug] Race condition in lib.py causes concurrent API calls to fail
4 participants