feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication #2655
Conversation
davidhcoe
changed the title
davidhcoe
changed the title
|
Updated test results after refactoring the tests:
|
|
This can wait until #2698 closes and then I will incorporate those changes in this PR. |
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks for the work! It sure would be nice to define a semantic contract for this behavior as part of the spec, but if/when that happens we can always change the implementation to conform to it.
My biggest concern around this change is a lack of clarity about concurrency issues. The API clearly incorporates a Task (which implies that concurrency is possible) and the implementations frequently use Tasks (if only because of the Google APIs) but there's a lot of shared state and no clear picture for where concurrency might end up happening or steps taken to guard shared data structures against it.
| /// </summary> | ||
| /// <param name="ex">The exception that occurs.</param> | ||
| /// <returns>True/False indicating a refresh is needed.</returns> | ||
| public bool TokenRequiresUpdate(Exception ex); |
There was a problem hiding this comment.
How is client code expected to use this?
There was a problem hiding this comment.
It feels like this class is serving as both the user-facing API and an internal-facing API. My vague expectation would be that there be one interface which allows the UpdateToken handler to be set and a different interface which generic credential-handling code uses to implement credential logic.
There was a problem hiding this comment.
I moved this to be an internal only API, but I don't think it can be split into two different APIs just because RetryManager looks at the ITokenProtectedResource to determine if the token needs to be updated then invokes the delegate for the update to occur.
I had been specifically concerned about a token expiring while we were downloading chunks in parallel, but it turns out the connector is not doing that. |
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! The delay doubling and the lack of retry on CreateReadSession are my two main points of feedback.
| } | ||
|
|
||
| private StructArray GetTableSchemas( | ||
| StructArray GetTableSchemas( |
There was a problem hiding this comment.
We should have a documented "preferred coding conventions". Much of the other C# code in this overall repo is explicitly using "private", and getting rid of it here has added a bunch of unrelated churn to this PR.
|
|
||
| ReadSession rs = new ReadSession { Table = table, DataFormat = DataFormat.Arrow }; | ||
| ReadSession rrs = readClient.CreateReadSession("projects/" + results.TableReference.ProjectId, rs, maxStreamCount); | ||
| ReadSession rrs = clientMgr.ReadClient.CreateReadSession("projects/" + results.TableReference.ProjectId, rs, maxStreamCount); |
There was a problem hiding this comment.
Should this have a retry? Given that there's a CreateReadSessionAsync I imagine it must be making a network request.
| } | ||
|
|
||
| await Task.Delay(delay); | ||
| delay *= 2; |
There was a problem hiding this comment.
I think this should stop doubling at some point. On the tenth iteration, we're waiting nearly 3.5 minutes. If it finishes around thirty seconds into the eleventh wait, then we still have to wait almost three more minutes before checking again.
There was a problem hiding this comment.
I'm not sure whether the new code delay += delay is supposed to be different than delay *= 2. Did you mean delay += initialDelayMilliseconds? Can I suggest an alternative of something like delay = Math.Min(2 * delay, 5000)?
…cation (apache#2655) - Adds support for users to login with their Entra / Azure AD account - Adds a retry concept to the driver that will check whether a token needs to be refreshed and then invoke a delegate so an outside caller can perform the token update. Will only go this path if the user has defined a handler for UpdateToken. - Includes long running tests to demonstrate the concept:  --------- Co-authored-by: David Coe <>
2 participants
Uh oh!
There was an error while loading. Please reload this page.