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

[feat] Adding Custom Authenticator and Http Conn Pool #81

Closed
wants to merge 2 commits into from
Closed

[feat] Adding Custom Authenticator and Http Conn Pool #81

wants to merge 2 commits into from

Conversation

andrefurlan-db
Copy link
Contributor

This PR adds:

  • the ability to pass in an Authenticator to the driver. If a token is passed, the PAT authenticator will be used
  • reusability of HTTP connections, including increasing MaxIdleConnsPerHost to 10.
  • better error message, including Databricks specific error headers.
  • fixed a few bugs in the way, namely on the retryable client and failure to close operation when context is canceled.

@andrefurlan-db andrefurlan-db added the enhancement New feature or request label Dec 16, 2022
@andrefurlan-db andrefurlan-db linked an issue Dec 16, 2022 that may be closed by this pull request
Improve http client set up #76
Open
@andrefurlan-db
WithAuthenticator
05ce8b2 
Signed-off-by: Andre Furlan <andre.furlan@databricks.com>
moderakh
moderakh reviewed Dec 16, 2022
View reviewed changes
auth/auth.go

import "net/http"

type Authenticator interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a public user facing API?

Copy link
Contributor Author

@andrefurlan-db andrefurlan-db Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is a public facing interface. Only needed if user wants to have their own implementation. Part of the auth package, not the dbsql package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrefurlan-db I am planning to write the decision doc we discussed sometime end of this week / early next week. as this is a change in the public surface area, can we wait to close on that decision before merging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is target for version 1.1.0. It won't be merged this year.

@andrefurlan-db
Copy link
Contributor Author

This is a v1.1.0 release PR

@andrefurlan-db
move auth here
77864ff 
Signed-off-by: Andre Furlan <andre.furlan@databricks.com>
nfx
nfx suggested changes Dec 23, 2022
View reviewed changes
auth/pat/pat.go
"github.com/pkg/errors"
)

type PATAuth struct {
Copy link

Choose a reason for hiding this comment

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

why do you need to re-implement auth, if there's a concurrent PR on integrating with Go SDK, that solves the auth problem in a unified way across different tools?

@moderakh moderakh self-requested a review December 28, 2022 16:34
moderakh
moderakh suggested changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

pending a one decision doc on the plan as a follow up internal discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfx nfx nfx requested changes

@moderakh moderakh moderakh requested changes

@rcypher-databricks rcypher-databricks rcypher-databricks approved these changes

@mattdeekay mattdeekay Awaiting requested review from mattdeekay

@susodapop susodapop Awaiting requested review from susodapop

@yunbodeng-db yunbodeng-db Awaiting requested review from yunbodeng-db yunbodeng-db is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve http client set up
4 participants
@andrefurlan-db @nfx @moderakh @rcypher-databricks