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

Support creating a driver directly without registering it #3

Merged
merged 1 commit into from
Dec 29, 2017

Conversation

sirwart
Copy link
Contributor

@sirwart sirwart commented Dec 28, 2017

In some cases it's useful to reuse drivers to prevent leaking drivers every
time a connection is opened. This allows library users to register the driver
themselves and reuse the registered name when necessary.

In some cases it's useful to reuse drivers to prevent leaking drivers every
time a connection is opened. This allows library users to register the driver
themselves and reuse the registered name when necessary.
@tejasmanohar
Copy link
Contributor

Thanks for the 3 PRs! Glad to see someone else is using go-athena. I'll review shortly.

@tejasmanohar
Copy link
Contributor

@sirwart This whole register ordeal feels hacky, but leaking drivers seems like a real problem if your app is connecting to lots of "Athenas" (e.g. a BI platform). Just to clarify, does the following snippet reflect how you intend to use this?

driver := athena.NewDriver()
sql.Register("athena", driver)
// do stuff with driver
// later,
newDriver := athena.NewDriveR() 
*driver = *newDriver

Mind adding a godoc comment about reusing the Driver, as this trick may help others, too?

@f2prateek
Copy link

Probably need a different name for that to work, otherwise you'd be calling register twice (once from your snippet and once from the init) twice which panics. https://godoc.org/database/sql#Register

@sirwart
Copy link
Contributor Author

sirwart commented Dec 28, 2017

Thanks Tejas, this driver helped me get going a lot faster than otherwise.

My solution for avoiding leaking is to register as athena-\<configuration hash\>, so there's a driver for every unique configuration rather than for every database connection opened. I've included my code below that's pretty complicated to talk about in the doc string. Pushing this logic into the library would require serializing aws.Config which I wasn't sure about, but now doesn't sound like a terrible idea. What do you think?

func (d athenaDriver) ConnectionStr(timeout int) (driverName, connectionStr string) {
	bytes, err := json.Marshal(d.connConfig)
	if err != nil {
		panic(err)
	}

	h := sha256.New()
	h.Write(bytes)
	hash := hex.EncodeToString(h.Sum(nil))
	driverName = fmt.Sprintf("athena-%s", hash)

	athenaConfigMutex.Lock()
	func() {
		defer athenaConfigMutex.Unlock()

		if athenaConfigs == nil {
			athenaConfigs = map[string]bool{}
		}
		if !athenaConfigs[hash] {
			creds := credentials.NewStaticCredentials(d.connConfig.AwsAccessKeyId, d.connConfig.AwsSecretAccessKey, "")
			session, err := session.NewSession(&aws.Config{
				Region:      d.connConfig.Region,
				Credentials: creds,
			})
			if err != nil {
				panic(err)
			}
			driverConfig := athena.Config{session, d.connConfig.Database, d.connConfig.StagingDir, 1 * time.Second}
			sql.Register(driverName, athena.NewDriver(&driverConfig))
			athenaConfigs[hash] = true
		}
	}()
	return
}

@tejasmanohar
Copy link
Contributor

tejasmanohar commented Dec 29, 2017

@sirwart That makes sense. Thanks for the context.

I'm afraid aws.Config has some unserializable options, like custom HTTP transports, but I'd be happy to merge support simple, popular options, like AWS key/secret, in the connection string. That would probably be simplest for your use case.

Regardless, let's go ahead and merge this in.

@tejasmanohar tejasmanohar merged commit 66c80a7 into segmentio:master Dec 29, 2017
@sirwart
Copy link
Contributor Author

sirwart commented Dec 29, 2017

Yea, good point. If you decide to go the adding options route, it'd probably be good to include region in addition to the AWS key/secret which would cover all the basics needed to connect to Athena.

The only other thing I can think of are the encryption options, but I haven't had to deal with them yet.

@sirwart sirwart deleted the driver-constructor branch December 29, 2017 18:22
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.

None yet

3 participants