code review 7819043: database/sql: document non-open of Open; add Ping

database/sql: document non-open of Open; add Ping

Fixes issue 4804

[bradfitz, 2013-03-14 19:41:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2013-03-14 19:45:11]

https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:271: // the dataSourceName: golang.org/issue/4804
update this comment?

very odd design, i must say

[bradfitz, 2013-03-14 19:56:49]

On Thu, Mar 14, 2013 at 12:45 PM, <r@golang.org> wrote:

>
> https://codereview.appspot.**com/7819043/diff/4001/src/pkg/**
>
database/sql/sql.go<https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> https://codereview.appspot.**com/7819043/diff/4001/src/pkg/**
>
database/sql/sql.go#newcode271<https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go#newcode271>
> src/pkg/database/sql/sql.go:**271: // the dataSourceName:
> golang.org/issue/4804
> update this comment?
>
> very odd design, i must say
>

It does means you can have it as a global variable and not pay for it until
you use it.

e.g. an App Engine global.

I figured it was easier to document the status quo rather than try to
invent a whole opt-in/opt-out mechanism for the behavior.

[r, 2013-03-14 20:00:39]

why don't you say that in the comments?

-rob

[bradfitz, 2013-03-14 20:06:07]

Because I don't want to guarantee that either, in case we let drivers do
checks in the future.

I'm just saying there are no guarantees now, but if what you care about is
health checks, use Ping.  You can't depend on Open for that.


On Thu, Mar 14, 2013 at 1:00 PM, Rob Pike <r@golang.org> wrote:

> why don't you say that in the comments?
>
> -rob
>

[r, 2013-03-14 20:09:01]

LGTM but consider naming

https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:282: // Ping returns any error contacting the
database.
// Ping attempts to establish a connection to the database and returns any
error.


maybe Ping is the wrong name, although it's nice and short. Connect?

[bradfitz, 2013-03-14 20:22:56]

Connect isn't right, though.  The database can be a local file, a single
TCP connection, UDP, shared memory, whatever.  And it might already be
connected.  Or it might've once connected and then died.

Ping says "check that you can reach it (for either the 1st or Nth time),
however it is reachable".


On Thu, Mar 14, 2013 at 1:09 PM, <r@golang.org> wrote:

> LGTM but consider naming
>
>
>
> https://codereview.appspot.**com/7819043/diff/4001/src/pkg/**
>
database/sql/sql.go<https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> https://codereview.appspot.**com/7819043/diff/4001/src/pkg/**
>
database/sql/sql.go#newcode282<https://codereview.appspot.com/7819043/diff/4001/src/pkg/database/sql/sql.go#newcode282>
> src/pkg/database/sql/sql.go:**282: // Ping returns any error contacting
> the database.
> // Ping attempts to establish a connection to the database and returns
> any error.
>
>
> maybe Ping is the wrong name, although it's nice and short. Connect?
>
>
https://codereview.appspot.**com/7819043/<https://codereview.appspot.com/7819...
>

[r, 2013-03-14 20:40:33]

my concern about "Ping" is that it implies it's something you do as a
keepalive, but unless i misunderstand it's really  part of (lowercase)
open.

-rob

[bradfitz, 2013-03-14 20:49:51]

how about we call it:

CheckAliveOrStillAlive


On Thu, Mar 14, 2013 at 1:40 PM, Rob Pike <r@golang.org> wrote:

> my concern about "Ping" is that it implies it's something you do as a
> keepalive, but unless i misunderstand it's really  part of (lowercase)
> open.
>
> -rob
>

[bradfitz, 2013-03-14 21:06:49]

*** Submitted as https://code.google.com/p/go/source/detail?r=332e552cd896 ***

database/sql: document non-open of Open; add Ping

Fixes issue 4804

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/7819043

[julienschmidt, 2013-03-14 21:10:56]

Or just "Check" or "Alive"

On 2013/03/14 20:49:51, bradfitz wrote:
> how about we call it:
> 
> CheckAliveOrStillAlive
> 
> 
> On Thu, Mar 14, 2013 at 1:40 PM, Rob Pike <mailto:r@golang.org> wrote:
> 
> > my concern about "Ping" is that it implies it's something you do as a
> > keepalive, but unless i misunderstand it's really  part of (lowercase)
> > open.
> >
> > -rob
> >