common/prpc, tools/cmd/cproto: prpc client

common/prpc, tools/cmd/cproto: prpc client

* common/prpc package implements a pRPC Client
* tools/cmd/cproto: generate NewXxxPRPCClient functions
* move gRPC code <-> HTTP status code mapping from server/prpc to
  common/prpc

R=dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/46e51273292d7a72a7169e6fe66314e645e0645f

[dnj, 2016-01-27 00:22:48]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, nodir@chromium.org

[dnj, 2016-01-27 00:22:49]

PTAL. Continued from: https://codereview.chromium.org/1605363002/

[nodir, 2016-01-27 00:24:47]

lgtm

[Vadim Sh., 2016-01-27 01:13:54]

lgtm with nits

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/client.go#n...
common/prpc/client.go:102: logging.Debugf(ctx, "RPC %s/%s.%s", c.Host,
serviceName, methodName)
host, serviceName and methodName is already in the fields...

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/client.go#n...
common/prpc/client.go:107: if err != nil {
detect context cancelation here? We don't want to retry it.

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/client.go#n...
common/prpc/client.go:124: return fmt.Errorf("failed to read response body: %s",
err)
this is most probably transient error too (e.g. connectivity error when reading
from the socket)

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/client.go#n...
common/prpc/client.go:196: if options.Insecure {
can we also do it by default for "localhost"? Very useful when testing stuff
against GAE dev server.

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/generate.go
File common/prpc/generate.go (left):

https://codereview.chromium.org/1637193002/diff/20001/common/prpc/generate.go...
common/prpc/generate.go:7: //go:generate go install
github.com/luci/luci-go/tools/cmd/cproto
:(

[dnj (Google), 2016-01-27 02:42:54]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-01-27 02:42:54]

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:88: // server using the HeaderTimeout heaer.
typo will fix.

[dnj (Google), 2016-01-27 02:46:39]

(vadimsh@, updated w/ Context-based timeout if you'd like to take another look).

[iannucci, 2016-01-27 02:55:48]

iannucci@chromium.org changed reviewers:
- dnj@google.com

[iannucci, 2016-01-27 02:55:50]

lgtm

https://codereview.chromium.org/1637193002/diff/40001/common/errors/transient.go
File common/errors/transient.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/errors/transient...
common/errors/transient.go:21: var _ Wrap = transientWrapper{}
var _ interface {
  Transient
  Wrap
} = transientWrapper{}

?

https://codereview.chromium.org/1637193002/diff/40001/common/errors/wrap.go
File common/errors/wrap.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/errors/wrap.go#n...
common/errors/wrap.go:8: type Wrap interface {
Wrapped?

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:88: // server using the HeaderTimeout heaer.
heaer typo

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:89: func (c *Client) Call(ctx context.Context,
serviceName, methodName string, in, out proto.Message, opts ...grpc.CallOption)
error {
this should probably just be CallOption: if we s/prpc/grpc, it would still work,
but if the user passes an actual grpc.CallOption here, it would fail at runtime.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:142: buf.Grow(int(res.ContentLength))
hm... what if this number is enormous? Can we cap it at 32MB?

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:164: return fmt.Errorf("HTTP %d: no gRPC code. Body: %s",
res.StatusCode, body)
what if it's not valid utf8? can we quote it?

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/codes.go
File common/prpc/codes.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/codes.go#ne...
common/prpc/codes.go:21: codes.Canceled:           http.StatusNoContent,
NoContent? That doesn't sound right... that's 204 ? Can Canceled ever be
returned by the server?

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/options.go
File common/prpc/options.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/options.go#...
common/prpc/options.go:51: panic(fmt.Errorf("non-pRPC call option %T is used
with pRPC client", co))
then you don't need this

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/timeout.go
File common/prpc/timeout.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/timeout.go#...
common/prpc/timeout.go:80: func EncodeTimeout(t time.Duration) string {
EncodeRPCTimeout?

[nodir, 2016-01-27 03:04:18]

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:40: HeaderTimeout = "X-Prpc-Grpc-Deadline"
was accidentally renamed to deadline? 
fwiu, usually "deadline" is an absolute value of time and timeout is a duration.
grpc timeout also called "grpc-timeout"

[dnj (Google), 2016-01-27 03:24:25]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-01-27 03:24:26]

PTAL at the content bounding code?

https://codereview.chromium.org/1637193002/diff/40001/common/errors/transient.go
File common/errors/transient.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/errors/transient...
common/errors/transient.go:21: var _ Wrap = transientWrapper{}
On 2016/01/27 02:55:49, iannucci wrote:
> var _ interface {
>   Transient
>   Wrap
> } = transientWrapper{}
> 
> ?

Done.

https://codereview.chromium.org/1637193002/diff/40001/common/errors/wrap.go
File common/errors/wrap.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/errors/wrap.go#n...
common/errors/wrap.go:8: type Wrap interface {
On 2016/01/27 02:55:49, iannucci wrote:
> Wrapped?

Agreed, done.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:89: func (c *Client) Call(ctx context.Context,
serviceName, methodName string, in, out proto.Message, opts ...grpc.CallOption)
error {
On 2016/01/27 02:55:49, iannucci wrote:
> this should probably just be CallOption: if we s/prpc/grpc, it would still
work,
> but if the user passes an actual grpc.CallOption here, it would fail at
runtime.

I don't really mind this. The fall-through is nice and this is centralized and
easy to change if we see a lot of people passing options in foolishly.

I turned this case into a soft failure instead of a panic, though.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:142: buf.Grow(int(res.ContentLength))
On 2016/01/27 02:55:49, iannucci wrote:
> hm... what if this number is enormous? Can we cap it at 32MB?

I've added a soft limit, default limits, and checks to ensure that we never read
beyond that limit.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:164: return fmt.Errorf("HTTP %d: no gRPC code. Body: %s",
res.StatusCode, body)
On 2016/01/27 02:55:49, iannucci wrote:
> what if it's not valid utf8? can we quote it?

Done.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/codes.go
File common/prpc/codes.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/codes.go#ne...
common/prpc/codes.go:21: codes.Canceled:           http.StatusNoContent,
On 2016/01/27 02:55:50, iannucci wrote:
> NoContent? That doesn't sound right... that's 204 ? Can Canceled ever be
> returned by the server?

Looks like this function isn't even used. Going to remove.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/options.go
File common/prpc/options.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/options.go#...
common/prpc/options.go:51: panic(fmt.Errorf("non-pRPC call option %T is used
with pRPC client", co))
On 2016/01/27 02:55:50, iannucci wrote:
> then you don't need this

Acknowledged.

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/timeout.go
File common/prpc/timeout.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/timeout.go#...
common/prpc/timeout.go:80: func EncodeTimeout(t time.Duration) string {
On 2016/01/27 02:55:50, iannucci wrote:
> EncodeRPCTimeout?

I dunno, there's only one timeout encoding so this is probably fine.

[dnj (Google), 2016-01-27 03:25:53]

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go
File common/prpc/client.go (right):

https://codereview.chromium.org/1637193002/diff/40001/common/prpc/client.go#n...
common/prpc/client.go:40: HeaderTimeout = "X-Prpc-Grpc-Deadline"
On 2016/01/27 03:04:18, nodir wrote:
> was accidentally renamed to deadline? 
> fwiu, usually "deadline" is an absolute value of time and timeout is a
duration.
> grpc timeout also called "grpc-timeout"

Done.

[dnj (Google), 2016-01-27 16:44:08]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2016-01-27 16:44:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
vadimsh@chromium.org, nodir@chromium.org, iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1637193002/#ps120001
(title: "Add some content length tests.")

[commit-bot: I haz the power, 2016-01-27 16:44:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1637193002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1637193002/120001

[commit-bot: I haz the power, 2016-01-27 16:47:58]

Description was changed from

==========
common/prpc, tools/cmd/cproto: prpc client

* common/prpc package implements a pRPC Client
* tools/cmd/cproto: generate NewXxxPRPCClient functions
* move gRPC code <-> HTTP status code mapping from server/prpc to
  common/prpc

R=dnj@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
common/prpc, tools/cmd/cproto: prpc client

* common/prpc package implements a pRPC Client
* tools/cmd/cproto: generate NewXxxPRPCClient functions
* move gRPC code <-> HTTP status code mapping from server/prpc to
  common/prpc

R=dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed:
https://github.com/luci/luci-go/commit/46e51273292d7a72a7169e6fe66314e645e0645f
==========

[commit-bot: I haz the power, 2016-01-27 16:47:59]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/46e51273292d7a72a7169e6fe66314e645e0645f