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.

Sign up for GitHub

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

Jump to bottom

Make grant_access_to thread safe #265

Closed
wants to merge 6 commits into from
Closed

Make grant_access_to thread safe #265

wants to merge 6 commits into from

Conversation

judahrand
Copy link

@judahrand judahrand commented Aug 11, 2022 •
edited
Loading

resolves #

Description

This PR aims to prevent the 412 PreConditionFailed error which is referenced here: dbt-labs/docs.getdbt.com#634

Fixes: #266

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have run changie new to create a changelog entry

@cla-bot cla-bot bot added the cla:yes label Aug 11, 2022
@judahrand judahrand force-pushed the make-grant-access-to-threadsafe branch from 219ed54 to 0cb0157 Compare August 11, 2022 10:13
@judahrand judahrand force-pushed the make-grant-access-to-threadsafe branch from 0cb0157 to d92e7bc Compare August 11, 2022 10:16
This was referenced Aug 11, 2022
Closed
Closed
@judahrand judahrand force-pushed the make-grant-access-to-threadsafe branch from d92e7bc to adf8c5b Compare August 11, 2022 10:29
@judahrand judahrand force-pushed the make-grant-access-to-threadsafe branch from adf8c5b to fe0c041 Compare August 11, 2022 10:42
@judahrand
Copy link
Author

@jtcohen6 Is this something which you think is worth merging / looking at?

@jtcohen6
Copy link
Contributor

@judahrand Thanks for the bump! I'm going to mark this as ready for review by the Core-Adapters engineering team. They have a lot on their plate right now, so it might be a bit before one of them can take a look. I definitely think this would represent an improvement over the status quo, until we can revisit authorized views more holistically in the future.

@jtcohen6 jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 19, 2022
@cwelton
Copy link

cwelton commented Sep 26, 2022

Is this moving forward? Does the solution need work?

Running into the #266 and wondering when it will get fixed.

@colin-rogers-dbt colin-rogers-dbt changed the title Make grant_on_access thread safe Make grant_access_to thread safe Oct 13, 2022
@judahrand
Copy link
Author

@colin-rogers-dbt is this merge-able?

@colin-rogers-dbt
Copy link
Contributor

hey @judahrand apologies on the delay, I've been digging into this issue and might have an alternate solution to consider here as well. Have you tested this/seen if it has a performance impact?

@Fleid Fleid mentioned this pull request Jan 27, 2023
Closed
@judahrand judahrand closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@McKnight-42 McKnight-42 Awaiting requested review from McKnight-42

@jtcohen6 jtcohen6 Awaiting requested review from jtcohen6

@colin-rogers-dbt colin-rogers-dbt Awaiting requested review from colin-rogers-dbt

Assignees
No one assigned
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1032] grant_access_to only works reliably in single threaded mode
6 participants
@judahrand @jtcohen6 @cwelton @colin-rogers-dbt @Fleid @McKnight-42