Google-cloud-go: all: document client connection behavior

[ahmetb]

EDIT: See https://github.com/GoogleCloudPlatform/google-cloud-go/issues/516#issuecomment-324173926 for the new direction for this bug.

It looks like datastore#Client.Close() closes the underlying connection. However while reading the doc:

1. I see that none of the example code with datastore.NewClient actually calls Close() on the client. Which makes me think maybe it

So in the godoc, I don't see any clear guidance on whether:

• the connection can be left open or should be closed.
• datastore.Client should be reused (i.e. it maintains a connection pool maybe?)

Perhaps clarifying these with some remarks, or fixing either the example code or the Close() godoc would help.

[rakyll]

/cc @adams-sarah

[ahmetb]

Actually found one example where it is closed. https://godoc.org/cloud.google.com/go/datastore#ex-package--Auth But there are other 2-3 more visible examples that it isn't closed.

[jba]

If a client has a Close method, you should consider it a heavyweight object. Reuse if possible, and call Close when you're done (unless your program is terminating).

We should document the Close method better.

[ahmetb]

@jba that'd be my gut feeling as well. However it's not immediately obvious to me. Moreover, what's not clear to me is: is Client doing connection pooling? If not, should I implement it myself, because I think my requests might be getting serialized if the client.conn is a single connection?

I found this to be a friction myself while developing an app on top of Datastore using Go, hence I’m reporting it. I think it would be helpful to document this explicitly. If the Client godoc would say "we do connection pooling for you, don't worry, just Close() when you're done", that'd be very ideal, but as you can understand, I still don’t know what the behavior is so my question still stands.

[ahmetb]

@jba do you have any answers to my question above?

[jba]

The cloud.google.com/go/datastore client does not do connection-pooling by default Use the WithGRPCConnectionPool option to create a pool.

[ahmetb]

Should we document this behavior? I think it's worth being verbose about which clients do pooling, whether users should close clients and such semantics.

[jba]

Agreed.

[jba]

So the action item for this bug is: for each package in this repo, document:

• That Client.Close should be called (if it exists)
• Whether the client does connection pooling on its own
• How to create an underlying connection pool if it doesn't

[broady]

I feel like all clients should do pooling. Why doesn't it?

Having "GRPC" in the option name feels bad to me. We should avoid exposing implementation details like what type of transport is used. Why not just "WithConnectionPoolSize(int)"?

[jadekler]

Hello! Some of our clients use http, and some use grpc. Here are more details about these usages:

http

• HTTP clients are golang net/http clients, which support pooling by caching existent connections for future re-use. This can be tweaked in the http transport by configuring MaxIdleConnsPerHost and MaxIdleConns. For the clients that use http, these options can be configured in an http client which can be passed in with WithHTTPClient.
• I believe that connections are used on a "first available" basis.
• I believe conns are spun down per client side IdleConnTimeout setting.

grpc

• Some gRPC clients use a custom pool resolver (instead of one of grpc's resolvers) with a specific pool size. For pubsub, as an example, the pool resolver is used and size is by default configured to GOMAXPROCS (as mentioned before), although the size can be changed with WithGRPCConnectionPool option. Some clients do not specific a resolver, which causes the passthrough resolver to be used (essentially pool size of 1). Users can force a pool to be used and set the size with the WithGRPCConnectionPool option.
• This "pool resolver" forces gRPC to open x transport connections to the address where x = poolSize, and then round robin requests between the conns, as opposed to the aforementioned "first available" basis.
• I believe that conns are spun down per server side KeepAlive setting.

Answering some questions in this thread:

• I'm not sure that we can offer a ubiquitous WithConnectionPool(size), since that guarantee isn't something we can make for http conns.
• All http clients implicitly use connection pooling.
• I'll dig in some more and take a look at what's happening for pooling across our grpc clients. There are some other pools floating around that I'm unfamiliar with. That said, it's safe to say that if you use WithGRPCConnectionPool, you will get pooling.
• I'll take a pass through our docs and see if we can clarify this setup.
• I believe all clients are intended heavy-weight; you should keep and use one client, instead of implementing pooling yourself.

[ahmetb]

@jadekler Thanks for the thorough work. It would be great to capture the gist of these deductions and filter those that are relevant to the users and include in package or Client godocs. For example, in storage.Client, I'd love to see something like "This client internally provides connection pooling.".

[jadekler]

SGTM, I'll make a doc pass on this.

[jadekler]

@ahmetb @broady Further investigation reveals that it's not easy to change HTTP transport settings, since supplying your own HTTP client causes yours to be used immediately without getting all the nice auth goodies that users normally get.

Not sure if any user cares much about this (in my experience most people are very OK with the default transport settings vis-à-vis caching conns), but thought I'd mention.

[ahmetb]

I'm completely fine with not being able to update the settings, what I'm more interested in is surfacing the underlying transport/pooling information on the Client godocs.

[jadekler]

Closed as of https://github.com/GoogleCloudPlatform/google-cloud-go/commit/dfdc5040dbf96e12b7ad6d8080d0c8bd380cb8f2