Add Google Cloud SQL support for App Engine. #167

dalinaum · 2013-11-08T18:34:17Z

It uses Cloud SQL as a database if it runs on Google App Engine with using cloudsql DSN.

appengine.go includes a build constraint for GAE to avoid an error outside App Engine.

julienschmidt · 2013-11-08T18:44:37Z

appengine.go

@@ -0,0 +1,29 @@
+// +build appengine


The header should be:

// Go MySQL Driver - A MySQL-Driver for Go's database/sql package // // Copyright 2013 The Go-MySQL-Driver Authors. All rights reserved. // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this file, // You can obtain one at http://mozilla.org/MPL/2.0/. // +build appengine package mysql

Same for the other file

dalinaum · 2013-11-08T19:09:32Z

OK. I'm fixing my commit. :)

dalinaum · 2013-11-09T08:26:12Z

I modified them all.

removed appenginedev.go.
added a var with a reference to dial function.

Check my new commit please.

julienschmidt · 2013-11-09T11:27:58Z

appengine.go

@@ -0,0 +1,30 @@
+// Go MySQL Driver - A MySQL-Driver for Go's database/sql package
+//
+// The driver should be used via the database/sql package:


This header is still wrong

dalinaum · 2013-11-09T11:39:01Z

Fixed the headers of appengine.go and driver.go. Thanks.

julienschmidt · 2013-11-09T12:10:49Z

driver.go

-	nd := net.Dialer{Timeout: mc.cfg.timeout}
-	mc.netConn, err = nd.Dial(mc.cfg.net, mc.cfg.addr)
+	if dialer != nil {
+		mc.netConn, err = dialer(mc.cfg)


dialershould be named dial since it is a function.

julienschmidt · 2013-11-09T12:15:57Z

I'm still not entirely happy with the current approach.
Does anyone have another idea for a clean approach?

arnehormann · 2013-11-09T12:32:21Z

Maybe. A var storing the dial function, initialized with the current
implementation. Overwrite it with init in appengine.go - none of this
exported. I'm grocery shopping right now, if the idea is unclear I can
flesh it out later.

Am 09.11.2013 13:16 schrieb "Julien Schmidt" notifications@github.com:

I'm still not entirely happy with the current approach.
Does anyone have another idea for a clean approach?

—
Reply to this email directly or view it on GitHub.

dalinaum · 2013-11-09T14:26:37Z

I tried another approach. I made a Dial map, dials to suppport multiple Dial functions. Adding multiple custom Dials is OK now. It doesn't lead to problems anymore.

If you have another approach let me know. Thanks.

arnehormann · 2013-11-11T09:10:31Z

I'd prefer to not export Dial and RegisterDial.
If it's driver-only, we can always export it later - we only have to change the driver.
If it's exported, we can never take it back.
I'd prefer to not get bug reports for external dialers - and motivate pull requests for different implementations.

My other problem with this is that we are not able to test the implementation with Travis. I think that's not really a big deal so far, but it can probably get out of hand.

Maybe we should also store the dial function in the connector, but that's a decision we can still make later.

So for this, LGTM if nothing gets exported and @julienschmidt agrees with me.

dalinaum · 2013-11-11T14:27:36Z

I fixed the commit to not export Dial and RegisterDial. It is replaced by dial and regiterDial. I agree with you.

I don't know how to test the implementation with Travis. Actually I don't know even Travis. I'm wondering what I have to do for advance.

I don't read other part of this library yet. I have to read more later.

Sorry for my poor englih and thanks for your review.

arnehormann · 2013-11-11T14:55:01Z

Travis-CI is an integration server. It checks each commit and runs tests on it - it even shows per commit if tests pass. The "build passing" banner underneath the logo in the README are a link to Travis.

It can not be integrated in this case because it doesn't run on Appengine.
Neither I nor - as far as I know - Julien have access to an Appengine, so we can't test whether this does what it says. It sure looks great, but we don't know.

arnehormann · 2013-11-11T14:57:01Z

driver.go

@@ -104,6 +112,10 @@ func (d *MySQLDriver) Open(dsn string) (driver.Conn, error) {
 	return mc, nil
 }

+func registerDial(protocol string, d dial) {


Drop this please, assign directly to the map in init.
There are no external users now and a direct assignment is shorter and simpler.

dalinaum · 2013-11-11T15:51:26Z

I fixed again. Dropeed registerDial and assigned directly to the map in init. I tested it locally, got an eror message, tested it on GAE and checked that it works correctly.

arnehormann · 2013-11-11T15:56:49Z

LGTM if @julienschmidt agrees

julienschmidt · 2013-11-11T17:47:45Z

driver.go

@@ -25,6 +25,9 @@ import (
 // This struct is exported to make the driver directly accessible.
 // In general the driver is used via the database/sql package.
 type MySQLDriver struct{}
+type dial func(*config) (net.Conn, error)


Insert a blank line before this type def to seperate it from the previous type def and comment, please

julienschmidt · 2013-11-11T17:49:20Z

For most users, the map will never be used. Therefore a lazy initialization of the map would be nice

dalinaum · 2013-11-11T17:51:42Z

Added a blank line before type dial.

Now, I have to modify an initialization of the map.

dalinaum · 2013-11-11T18:23:45Z

Applied a lazy initialization.

julienschmidt · 2013-11-11T18:25:27Z

appengine.go

+)
+
+func init() {
+	dials = make(map[string]dial)


if dials == nil { dials = make(map[string]dial) }

dalinaum · 2013-11-11T18:38:07Z

I fixed them as you reviewd. Removed an unnecessay type defines and added a condition statement to check if dials is initialized.

dalinaum · 2013-11-13T17:46:27Z

Please let me know what I have to do.

julienschmidt · 2013-11-13T17:49:03Z

wait 😃

I don't have time to review you code until tomorrow.

julienschmidt · 2013-11-14T10:27:06Z

driver.go

@@ -26,6 +26,8 @@ import (
 // In general the driver is used via the database/sql package.
 type MySQLDriver struct{}

+var dials map[string]func(*config) (net.Conn, error)


Sorry for changing my opinion every day... please reintroduce a type for the dial funcs:

type dialFunc func(*config) (net.Conn, error) var dials map[string]dialFunc

julienschmidt · 2013-11-14T10:30:32Z

driver.go

+	} else {
+		nd := net.Dialer{Timeout: mc.cfg.timeout}
+		mc.netConn, err = nd.Dial(mc.cfg.net, mc.cfg.addr)
+	}


if dial, ok := dials[mc.cfg.net]; ok { mc.netConn, err = dial(mc.cfg) } else { nd := net.Dialer{Timeout: mc.cfg.timeout} mc.netConn, err = nd.Dial(mc.cfg.net, mc.cfg.addr) }

Reading from a nil-map is fine, you don't need to check if the map is nil first
http://play.golang.org/p/tBq3LMI0hc

I didn't know this technique. It's quite interesting. I have to apply it.

It uses Cloud SQL as a database if it runs on Google App Engine with using cloudsql DSN. appengine.go includes a build constraint for Google App Engine (GAE) (http://blog.golang.org/the-app-engine-sdk-and-workspaces-gopath) to avoid an error outside App Engine and provide an exception. Using Google Cloud SQL with App Engine GO SDK is not supported yet. It is a SDK limitation.

dalinaum · 2013-11-14T12:40:24Z

Thanks for your review. I pushed a new commit.

julienschmidt · 2013-11-14T13:27:51Z

LGTM now.
Thanks for the PR and your patience!

Add Google Cloud SQL support for App Engine.

julienschmidt reviewed Nov 8, 2013
View reviewed changes

julienschmidt reviewed Nov 9, 2013
View reviewed changes

arnehormann reviewed Nov 11, 2013
View reviewed changes

julienschmidt reviewed Nov 11, 2013
View reviewed changes

appengine.go

)

func init() {

dials = make(map[string]dial)

Copy link

Member

julienschmidt Nov 11, 2013

Choose a reason for hiding this comment

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

if dials == nil { dials = make(map[string]dial) }

julienschmidt reviewed Nov 14, 2013
View reviewed changes

julienschmidt added a commit that referenced this pull request Nov 14, 2013

Merge pull request #167 from dalinaum/cloudsql

c418c1b

Add Google Cloud SQL support for App Engine.

julienschmidt merged commit c418c1b into go-sql-driver:master Nov 14, 2013

julienschmidt added this to the v1.2 milestone Apr 1, 2014

julienschmidt removed code review labels Apr 1, 2014

Add Google Cloud SQL support for App Engine. #167

Add Google Cloud SQL support for App Engine. #167

Uh oh!

Conversation

dalinaum commented Nov 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalinaum commented Nov 8, 2013

Uh oh!

dalinaum commented Nov 9, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalinaum commented Nov 9, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienschmidt commented Nov 9, 2013

Uh oh!

arnehormann commented Nov 9, 2013

Uh oh!

dalinaum commented Nov 9, 2013

Uh oh!

arnehormann commented Nov 11, 2013

Uh oh!

dalinaum commented Nov 11, 2013

Uh oh!

arnehormann commented Nov 11, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalinaum commented Nov 11, 2013

Uh oh!

arnehormann commented Nov 11, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienschmidt commented Nov 11, 2013

Uh oh!

dalinaum commented Nov 11, 2013

Uh oh!

dalinaum commented Nov 11, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalinaum commented Nov 11, 2013

Uh oh!

dalinaum commented Nov 13, 2013

Uh oh!

julienschmidt commented Nov 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalinaum commented Nov 14, 2013

Uh oh!

julienschmidt commented Nov 14, 2013

Uh oh!

Uh oh!