It defaults to using net.Dial instead of using DialTimeout with some sensible timeout like 1 second by default.
Should we change the default? or should we just change everywhere we use it?

@PatrickDropbox @rajatgoel

Can someone help us understand how we are supposed to insert a mysql DATE with an InsertStatement?

Currently we have: insertStatement.Add(sqlbuilder.Literal(time.Now())) which will escape the date with an extra set of double quotes which makes MySQL complain with: Error 1292: Incorrect date value: ''2014-08-20 11:17:53''

I feel like we aren't understanding something critical to inserting dates but so far everything else is working as designed.

BTW, we have modified (hacked) the sqltypes.go line 233 to not add the extra quotes around the time.Time value in which case our MySQL insert works fine.

Other than a couple of passing references in column.go and doc.go, it's not clear that sqlbuilder is meant to be run against mysql. Postgres barfs at least on the backticks for quoting. I have not tested it against sqlite.

It would be good to make this very clear in the package docs and README at a minimum. Do you have plans or want to support additional SQL flavors in the future?

Benchmark results:

Fake dealer:
BenchmarkGetConnection-16           	 1492227	       803.2 ns/op
BenchmarkGetConnectionMaxIdle-16    	 1888982	       631.4 ns/op
Real dealer:
BenchmarkGetConnection-16           	    9052	    113356 ns/op
BenchmarkGetConnectionMaxIdle-16    	 1901071	       626.9 ns/op

Code example:

func BenchmarkGetConnection(b *testing.B) {
        // dialer := fakeDialer{}
        // mockClock := time2.MockClock{}

        options := ConnectionOptions{
                // MaxIdleConnections: 1,
                // Dial:               dialer.FakeDial,
                // NowFunc:            mockClock.Now,
        }

        pool := NewSimpleConnectionPool(options)
        pool.Register("tcp", "localhost:11211")
        b.ResetTimer();
        for i := 0; i < b.N; i++ {
                c, err := pool.Get("tcp", "localhost:6379")
                if err != nil {
                        b.Fatal(err)
                }
                c.ReleaseConnection()
        }
}

func BenchmarkGetConnectionMaxIdle(b *testing.B) {
        // dialer := fakeDialer{}
        // mockClock := time2.MockClock{}

        options := ConnectionOptions{
                MaxIdleConnections: 1,
                // Dial:               dialer.FakeDial,
                // NowFunc:            mockClock.Now,
        }

        pool := NewSimpleConnectionPool(options)
        pool.Register("tcp", "localhost:11211")
        b.ResetTimer();
        for i := 0; i < b.N; i++ {
                c, err := pool.Get("tcp", "localhost:6379")
                if err != nil {
                        b.Fatal(err)
                }
                c.ReleaseConnection()
        }
}

I noticed that the Memcached binary protocol client provided by this library performs checks on the keys and disallows certain byte values, perhaps to maintain compatibility with the ASCII protocol:

Could this behavior be made configurable? The documentation for the Memcached binary protocol (https://github.com/memcached/memcached/wiki/BinaryProtocolRevamped) doesn't seem to impose these restrictions, and empirically it does seem possible to set and retrieve keys that this client considers invalid (such as keys with spaces).

There's a race condition on the http.Request created in func (s *SimplePoolSuite) TestHTTP(c *C):

req, err := http.NewRequest("GET", "/", nil)
// ...
for i := 0; i < count; i++ {
    go func() {
        _, err = pool.Do(req)
        // ...
    }()
}

The race occurs when *SimplePool.Do changes the URL scheme of the request (which is shared):

func (pool *SimplePool) Do(req *http.Request) (resp *http.Response, err error) {
    // ...
    if pool.params.UseSSL {
        req.URL.Scheme = "https"
    } else {
        req.URL.Scheme = "http"
    }
    // ...
}

Similarly for (same func):

if pool.params.HostHeader != nil {
    req.URL.Host = *pool.params.HostHeader
} else {
    req.URL.Host = pool.addr
}

Running go test ./... -race on the root package (more than one are found at this time):

WARNING: DATA RACE
Write by goroutine 414:
  github.com/dropbox/godropbox/net2/http2.(*SimplePool).Do()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool.go:103 +0x652
  github.com/dropbox/godropbox/net2/http2.func·008()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:36 +0x9d

Previous write by goroutine 83:
  github.com/dropbox/godropbox/net2/http2.(*SimplePool).Do()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool.go:103 +0x652
  github.com/dropbox/godropbox/net2/http2.func·008()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:36 +0x9d

Goroutine 414 (running) created at:
  github.com/dropbox/godropbox/net2/http2.(*SimplePoolSuite).TestHTTP()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:39 +0x45e
  runtime.call16()
      /Users/antoine/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /Users/antoine/go/src/pkg/reflect/value.go:411 +0xed
  gopkg.in/check%2ev1.func·003()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:753 +0x51b
  gopkg.in/check%2ev1.func·001()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:648 +0xf4

Goroutine 83 (running) created at:
  github.com/dropbox/godropbox/net2/http2.(*SimplePoolSuite).TestHTTP()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:39 +0x45e
  runtime.call16()
      /Users/antoine/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /Users/antoine/go/src/pkg/reflect/value.go:411 +0xed
  gopkg.in/check%2ev1.func·003()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:753 +0x51b
  gopkg.in/check%2ev1.func·001()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:648 +0xf4

and

WARNING: DATA RACE
Write by goroutine 41:
  github.com/dropbox/godropbox/net2/http2.(*SimplePool).Do()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool.go:108 +0x5d5
  github.com/dropbox/godropbox/net2/http2.func·008()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:36 +0x9d

Previous write by goroutine 83:
  github.com/dropbox/godropbox/net2/http2.(*SimplePool).Do()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool.go:108 +0x5d5
  github.com/dropbox/godropbox/net2/http2.func·008()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:36 +0x9d

Goroutine 41 (running) created at:
  github.com/dropbox/godropbox/net2/http2.(*SimplePoolSuite).TestHTTP()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:39 +0x45e
  runtime.call16()
      /Users/antoine/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /Users/antoine/go/src/pkg/reflect/value.go:411 +0xed
  gopkg.in/check%2ev1.func·003()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:753 +0x51b
  gopkg.in/check%2ev1.func·001()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:648 +0xf4

Goroutine 83 (running) created at:
  github.com/dropbox/godropbox/net2/http2.(*SimplePoolSuite).TestHTTP()
      /Users/antoine/gocode/src/github.com/dropbox/godropbox/net2/http2/simple_pool_test.go:39 +0x45e
  runtime.call16()
      /Users/antoine/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /Users/antoine/go/src/pkg/reflect/value.go:411 +0xed
  gopkg.in/check%2ev1.func·003()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:753 +0x51b
  gopkg.in/check%2ev1.func·001()
      /Users/antoine/gocode/src/gopkg.in/check.v1/check.go:648 +0xf4

Does the mysql binlog package work for mysql 8 despite the readme saying max v5.6?

I'm a little concerned about using your memcached library on AWS without having read/write timeouts. I know I can do this by supplying a custom Dial function in ConnectionOptions that creates an instance of a custom net.Conn-implementing struct that wraps net.Conn and calls SetReadDeadline before Read, etc... but that seems a bit hacky. Especially concerning that struct's SetDeadline methods: should I return an error or pass the deadlines through to net.Conn? The code in ManagedConnImpl seems to imply that the owner connection pool reserves the right to set deadlines, so returning an error might cause issues. On the other hand, passing the deadlines through would break the contract of the net.Conn interface, since they would be re-set before every Read/Write call.

I think it would be simple to add optional explicit read/write timeouts and would be willing to send a pull request with tests, if you reply to this issue saying you'd be willing to merge it.

It would be relatively simple to add:

To the ConnectionOptions struct I would add

    ReadTimeout time.Duration
    WriteTimeout time.Duration

These timeouts would be passed to NewManagedConn and stored in ManagedConnImpl. In ManagedConnImpl's Read method, if ReadTimeout > 0, I'd call SetReadDeadline before calling conn.Read, and similarly for the Write method.

Am I missing anything? Does this sound reasonable?

The file godropbox/singleton/singleton.go contains an incorrect synchronization pattern. It is assumed that s.data will always be filled correctly when s.initialized == true, but this is a false assumption. For more information, see the section on Incorrect synchronization at https://golang.org/ref/mem.

Either document that the Get() method might (theoretically) return garbage 😬 or use the sync/atomic package. For inspiration look at sync.Once (http://golang.org/src/sync/once.go?s=1139:1166#L25). 😄

When net2.GetLocalIPs is called on a Windows (amd64 only platform tested currently) machine, the call silently fails without providing any LocalIPs nor an error.

Hi, guys!
There might be a bug, which seem to be ingore some resource without calling Close function in SimpleResourcePool.getIdleHandle

	if len(p.idleHandles) > 0 {
		p.idleHandles = []*idleHandle{}
	}

If goes here, it means there's no handle can be used, but why don't close them, just ingore?
I think it might be this:

	if len(p.idleHandles) > 0 {
		toClose = p.idleHandles
		p.idleHandles = []*idleHandle{}
	}

Thanks!

In net2/http2 there is a file named test_utils.go, but needs to be utils_test.go.
Thanks and kudos for the useful library!

Would you be open to a pull-request adding support for customizing the timeouts on individual memcache requests? For example, 10ms for Get() and 100ms for a later Get().

Since this is a larger change, I wanted to check with you first before digging into this.

Rough Proposal:
Pass deadline using context.Context. To keep API compatibility, introduce new interfaces for this purpose: ContextClient, ContextClientShard, ContextManagedConn etc. Have existing implementations pass context.Background() to the new methods. It should be possible to leave the existing API unchanged.

Please let me know if you'd be open to such a change, or if there's a different approach you'd prefer me to take here.

Thanks in advance,
Ben Karas
Google

Hi, I'm using a net2 ConnectionPool, which was created using NewMultiConnectionPool. I cannot Unregister connections after I Register them. It looks like connectionPoolImpl's Unregister function does not call the underlying Unregister function: https://github.com/dropbox/godropbox/blob/f0626942059b/net2/base_connection_pool.go#L114

Seems like the fix is pretty simple, but I don't have permission to create a new remote branch to submit a PR.

This call to panic can be avoided if the func accepts a uint instead of an int as a param.

It returns GtidNextStatement so I would expect its name to be NewGtidNextStatement.