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

Issue 1605363002: common/prpc, tools/cmd/cproto: prpc client (Closed)

Created:
4 years, 11 months ago by nodir

Modified:
4 years, 10 months ago

Reviewers:
dnj, Vadim Sh., iannucci, estaab, dnj (Google)

CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, andrew.wang, todd, tandrii+luci-go_chromium.org, M-A Ruel

Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-go@master

Target Ref:
refs/heads/master

Project:
luci-go

Visibility:
Public.

More Reviews

Description

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=

Patch Set 1 : #

Patch Set 2 : use text template for ast generation #

Total comments: 54

Patch Set 3 : addressed comments #

Total comments: 18

Patch Set 4 : #

Total comments: 1

Patch Set 5 : rebased and addressed comments #

Patch Set 6 : rebased and addressed comments #

Created: 4 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1279 lines, -319 lines)			Patch
M	common/errors/transient.go	View		1 chunk	+7 lines, -0 lines	0 comments	Download
A	common/errors/wrap.go	View		1 chunk	+33 lines, -0 lines	0 comments	Download
M	common/logging/fields.go	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
A	common/prpc/client.go	View	1 2 3 4 5	1 chunk	+223 lines, -0 lines	0 comments	Download
A	common/prpc/client_test.go	View	1 2 3 4	1 chunk	+200 lines, -0 lines	0 comments	Download
A	common/prpc/codes.go	View		1 chunk	+84 lines, -0 lines	0 comments	Download
M	common/prpc/doc.go	View	1 2 3 4	2 chunks	+1 line, -4 lines	0 comments	Download
A +	common/prpc/generate.go	View		1 chunk	+0 lines, -1 line	0 comments	Download
A +	common/prpc/helloworld_test.proto	View		1 chunk	+1 line, -7 lines	0 comments	Download
A	common/prpc/helloworld_test.pb_test.go	View	1 2	1 chunk	+60 lines, -0 lines	0 comments	Download
A	common/prpc/options.go	View	1 2 3	1 chunk	+83 lines, -0 lines	0 comments	Download
M	common/retry/limited.go	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	common/retry/transient.go	View	1 2 3 4	1 chunk	+4 lines, -0 lines	0 comments	Download
M	server/prpc/auth.go	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	server/prpc/decoding.go	View	1 2 3 4	2 chunks	+3 lines, -3 lines	0 comments	Download
M	server/prpc/decoding_test.go	View	1 2 3 4	5 chunks	+5 lines, -5 lines	0 comments	Download
M	server/prpc/encoding.go	View	1 2 3 4	7 chunks	+50 lines, -100 lines	0 comments	Download
M	server/prpc/encoding_test.go	View	1 2 3 4	3 chunks	+10 lines, -48 lines	0 comments	Download
M	server/prpc/error.go	View	1 2 3 4	1 chunk	+14 lines, -26 lines	0 comments	Download
M	server/prpc/error_test.go	View	1 2 3 4	1 chunk	+0 lines, -34 lines	0 comments	Download
M	server/prpc/method.go	View	1 2 3 4	2 chunks	+27 lines, -54 lines	0 comments	Download
A	server/prpc/response.go	View	1 2 3 4	1 chunk	+75 lines, -0 lines	0 comments	Download
A	server/prpc/response_test.go	View	1 2 3 4	1 chunk	+92 lines, -0 lines	0 comments	Download
M	server/prpc/server.go	View	1 2 3 4	7 chunks	+45 lines, -9 lines	0 comments	Download
M	server/prpc/server_test.go	View	1 2 3 4	4 chunks	+24 lines, -1 line	0 comments	Download
M	server/prpc/service.go	View	1 2 3 4	1 chunk	+0 lines, -4 lines	0 comments	Download
M	tools/cmd/cproto/testdata/helloworld/test.pb.golden	View	1	2 chunks	+17 lines, -0 lines	0 comments	Download
M	tools/cmd/cproto/testdata/importGoogle/test.pb.golden	View	1	2 chunks	+26 lines, -0 lines	0 comments	Download
M	tools/cmd/cproto/testdata/retrain.sh	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	tools/cmd/cproto/testdata/twoFiles/1.pb.golden	View	1	2 chunks	+17 lines, -0 lines	0 comments	Download
M	tools/cmd/cproto/testdata/twoFiles/2.pb.golden	View	1	2 chunks	+8 lines, -0 lines	0 comments	Download
M	tools/cmd/cproto/transform.go	View	1	4 chunks	+161 lines, -16 lines	0 comments	Download

Messages

Total messages: 14 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

nodir

Switched from manual AST construction to a text template for client code generation.

4 years, 11 months ago (2016-01-20 22:57:37 UTC) #5

dnj

Looks pretty good in general. Handful of comments. https://chromiumcodereview.appspot.com/1605363002/diff/80001/common/prpc/client.go File common/prpc/client.go (right): https://chromiumcodereview.appspot.com/1605363002/diff/80001/common/prpc/client.go#newcode36 common/prpc/client.go:36: func ...

4 years, 11 months ago (2016-01-21 07:44:28 UTC) #8

nodir

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go File common/prpc/client.go (right): https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#newcode36 common/prpc/client.go:36: func NewClient(host string) *Client { On 2016/01/21 07:44:27, dnj ...

4 years, 11 months ago (2016-01-22 00:47:24 UTC) #9

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:36: func NewClient(host string) *Client {
On 2016/01/21 07:44:27, dnj wrote:
> This seems unnecessary, since "Client" has no private state to initialize.
Let's
> be cool like "http.Client"!

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:46: options = *DefaultOptions()
On 2016/01/21 07:44:28, dnj wrote:
> DefaultOptions generates a new instance anyway. Cloning it again seems
> unnecessary.
> 
> var options *Options
> if c.Options != nil {
>   oc := *c.Options
>   options = &oc
> } else {
>   options = DefaultOptions()
> }

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:68: fields := logging.Fields{
On 2016/01/21 07:44:28, dnj wrote:
> Fields layer naturally via "SetFields". ctx = logging.SetFields(ctx,
> logging.Fields{...}), then you basically get this for free with a lot less
code.
> All future calls logging.Errorf(ctx, "ohaithere") will inherit the installed
> fields.
> 
> Granted it costs a Context derivation but ... who cares?

ah, I see. Done, but I don't see much  code-size savings. Am I using fields
right?

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:75: onRetry := func(err error, sleepTime time.Duration) {
On 2016/01/21 07:44:27, dnj wrote:
> I personally like to inline this w/ the retry.Retry call, but up to you.

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:76: fields := fields.Copy(logging.Fields{
On 2016/01/21 07:44:27, dnj wrote:
> (Layering fields is a lot cleaner than explicitly copying them IMO.)

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:83: client.Transport = transport.Get(ctx)
On 2016/01/21 07:44:27, dnj wrote:
> I seriously dislike embedding Transport in context. 

If it is that serious, please start a discussion in the ML to remove the
transport package.

> Why not just make it a field
> of Client? Could even make the http.Client a field of Client.

Done. Didn't embed as namelessly because didn't want to "inherit" all of its
methods.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:85: fields.Infof(ctx, "RPC %s/%s.%s", c.Host, serviceName,
methodName)
On 2016/01/21 07:44:27, dnj wrote:
> This might be a bit wordy even for info level. Maybe debug?

Done. It is wordy indeed, we use info in
https://github.com/luci/luci-py/blob/master/appengine/components/components/n...

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:88: req.Body = ioutil.NopCloser(bytes.NewBuffer(reqBody))
On 2016/01/21 07:44:27, dnj wrote:
> bytes.NewReader is more lightweight and appropriate here.

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:91: return errors.WrapTransient(fmt.Errorf("failed to send
request: %s", err))
On 2016/01/21 07:44:28, dnj wrote:
> IMO you should handle transient errors more surgically. Maybe >= 500?

status is not available if err != nil

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:100: if res.StatusCode >= 300 {
On 2016/01/21 07:44:27, dnj wrote:
> Maybe http.StatusMultipleChoices instead of 300?

name "MultipleChoises" does not imply it is the first error status code. We
could have "firstBadStatus" constant (see also these markers in
https://golang.org/src/go/token/token.go#L22), but I believe HTTP status codes
are uniquos enough to know that bad statuses start with 300

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:105: buf, err := ioutil.ReadAll(res.Body)
On 2016/01/21 07:44:27, dnj wrote:
> Since you're doing this in a retry loop, you could optimize allocation via
> "bytes.Buffer{}":
> 
> // (Outside of loop)
> respBuf := bytes.Buffer{}
> 
> // (Here)
> respBuf.Reset()
> if err := respBuf.ReadFrom(res.Body); err != nil { ... }
> respBuf.Bytes()
> 
> Also if the server sends Content-Length, you can pre-allocate the Buffer:
> buffer.Grow(size) ... and use an io.LimitedReader to enforce it.

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:107: return errors.WrapTransient(fmt.Errorf("failed to
read response body: %s", err))
On 2016/01/21 07:44:27, dnj wrote:
> Why is a failure to read body transient? This would only happen if TCP is
> interrupted 

Isn't TCP interruption transient? In other words, if we repeat the request we
may get lucky.

> or if there is an error in the content decoding, both of which seem
> like non-transient.

What decoding you are talking about? Isn't this raw bytes?

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:118: fields[logging.ErrorKey] = err
On 2016/01/21 07:44:27, dnj wrote:
> If you set fields, you can make this:
> logging.WithError(err).Warningf(ctx, ...).

Done

> Athough this should probably be logging.Errorf. 

Errors are quite severe: in ereporter2 land, a service owner receives a report
every hour if errors are logged. A failed RPC may be less severe than that, but
nothing prevents the user to print errors if he/she believes a failed RPC is
that severe.

> And you don't need to embed the
> error in the message when you are using it as a field.

I prefer to control how it actually looks in the logs, given the number of
fields we have already. I want to avoid the situation when a human has hard time
parsing actual error message from printed 3 fields.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:152: req.Header.Set("User-Agent", options.UserAgent)
On 2016/01/21 07:44:27, dnj wrote:
> Endpoints clients have a default user agent (e.g., "pRPC Client 1.0") and
append
> the optional user-supplied UserAgent string to that. WDYT?

ah right, forgot about that

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:165: const maxBody = 256
On 2016/01/21 07:44:27, dnj wrote:
> Perhaps this could be a Client setting? That way, a user would have a method
of
> enlarging it if they actually want to see the whole response.

Done.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:174: const noCode = 0xffffffff
On 2016/01/21 07:44:27, dnj wrote:
> Rather than use a sentinel value, just use a boolean: hasCode := false
> 
> ...
> if !hasCode {
> 
> }

not relevant any more

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:177: const headerName = "X-Prpc-Grpc-Code"
On 2016/01/21 07:44:28, dnj wrote:
> Shared constant w/ server?

Added a constant

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:180: logging.Warningf(c, "could not parse %s header: %s",
headerName, err)
On 2016/01/21 07:44:28, dnj wrote:
> Capitalize logging messages. 

Done

> Use fields:
> 
> logging.Fields{
>   logging.ErrorKey: err,
>   "headerName": headerName,
> }.Warningf(c, ...)

used a field for err.
I %s the header name only to use the constant. It won't change, so it should not
be in the fields.
 
> If a code header is supplied, but the value can't be parsed, I don't think you
> should default to HTTP status. I think you should use codes.Unknown.

I agree. Done

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/codes.go#ne...
common/prpc/codes.go:19: var codeToStatus = map[codes.Code]int{
On 2016/01/21 07:44:28, dnj wrote:
> Why do we need a full mapping? We have the status code in the header unless
the
> pRPC server couldn't be reached. We should only have to map things sent
outside
> of pRPC Server (handful of 400s, 500s).

the {code->status} map is used in server/prpc. I've moved it from there to keep
the mapping in one file

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/options.go#...
common/prpc/options.go:57: Apply func(*Options)
On 2016/01/21 07:44:28, dnj wrote:
> Should this be internal "apply"?

I don't mind

https://codereview.chromium.org/1605363002/diff/80001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1605363002/diff/80001/server/prpc/encoding.go...
server/prpc/encoding.go:29: headerGRPCCode = "X-Prpc-Grpc-Code"
On 2016/01/21 07:44:28, dnj wrote:
> Make this exported and reuse it in client? No point in repeating it.

Done, but vice-versa: common cannot import server

dnj

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go File common/prpc/client.go (right): https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#newcode68 common/prpc/client.go:68: fields := logging.Fields{ On 2016/01/22 00:47:23, nodir wrote: > ...

4 years, 11 months ago (2016-01-22 01:16:27 UTC) #10

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:68: fields := logging.Fields{
On 2016/01/22 00:47:23, nodir wrote:
> On 2016/01/21 07:44:28, dnj wrote:
> > Fields layer naturally via "SetFields". ctx = logging.SetFields(ctx,
> > logging.Fields{...}), then you basically get this for free with a lot less
> code.
> > All future calls logging.Errorf(ctx, "ohaithere") will inherit the installed
> > fields.
> > 
> > Granted it costs a Context derivation but ... who cares?
> 
> ah, I see. Done, but I don't see much  code-size savings. Am I using fields
> right?

Looks like you are.

It's less about code size changes and more reliving the burden of management
from you. You can now assume these fields will always be there and move on with
your life.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:91: return errors.WrapTransient(fmt.Errorf("failed to send
request: %s", err))
On 2016/01/22 00:47:24, nodir wrote:
> On 2016/01/21 07:44:28, dnj wrote:
> > IMO you should handle transient errors more surgically. Maybe >= 500?
> 
> status is not available if err != nil

This sounds like a hard failure to me then, not a transient one.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:100: if res.StatusCode >= 300 {
On 2016/01/22 00:47:24, nodir wrote:
> On 2016/01/21 07:44:27, dnj wrote:
> > Maybe http.StatusMultipleChoices instead of 300?
> 
> name "MultipleChoises" does not imply it is the first error status code. We
> could have "firstBadStatus" constant (see also these markers in
> https://golang.org/src/go/token/token.go#L22), but I believe HTTP status codes
> are uniquos enough to know that bad statuses start with 300

Actually now that I think about it, you control the server, so you should
whitelist specific success codes. Maybe if StatusCode != OK? OK, NoContent?

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:107: return errors.WrapTransient(fmt.Errorf("failed to
read response body: %s", err))
On 2016/01/22 00:47:24, nodir wrote:
> On 2016/01/21 07:44:27, dnj wrote:
> > Why is a failure to read body transient? This would only happen if TCP is
> > interrupted 
> 
> Isn't TCP interruption transient? In other words, if we repeat the request we
> may get lucky.

It depends why the interruption happened. e.g, if there was a data error in the
HTTP response, it might not be transient. Regardless, the TCP/IP protocol will
take care of any transient cases of this (data corruption, interruption, etc.).
I don't think this is an application layer transient failure case.

> > or if there is an error in the content decoding, both of which seem
> > like non-transient.
> 
> What decoding you are talking about? Isn't this raw bytes?

res.Body's reader wraps GZIP and chunk decoding from the raw HTTP response. It
will return the raw bytes on success, but can fail if either of those decodings
don't work.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:180: logging.Warningf(c, "could not parse %s header: %s",
headerName, err)
On 2016/01/22 00:47:23, nodir wrote:
> On 2016/01/21 07:44:28, dnj wrote:
> > Capitalize logging messages. 
> 
> Done
> 
> > Use fields:
> > 
> > logging.Fields{
> >   logging.ErrorKey: err,
> >   "headerName": headerName,
> > }.Warningf(c, ...)
> 
> used a field for err.
> I %s the header name only to use the constant. It won't change, so it should
not
> be in the fields.
>  
> > If a code header is supplied, but the value can't be parsed, I don't think
you
> > should default to HTTP status. I think you should use codes.Unknown.
> 
> I agree. Done

Acknowledged.

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:33: HeaderGrpcCode = "X-Prpc-Grpc-Code"
nit: HeaderGRPCCode?

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:120: bodySize := c.ErrBodySize
Move this logic into responseErr and make responseErr a Client member function?

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:131: buf.Grow(int(req.ContentLength))
<res>.ContentLength** (and above)

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:134: if _, err = io.Copy(&buf, body); err != nil {
io.Copy reads in fixed-size chunks. You should use buf.ReadFrom, which reads in
buf-sized chunks if available. This would result in potentially zero
reallocations if the ContentLength was supplied and better performance in retry
cases.

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:149: }.Warningf(ctx, "RPC failed transiently. Will retry
in %s", sleepTime)
nit: Since you're emitting sleepTime as a Field, you don't need to repeat it in
the error string.

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/codes.go#n...
common/prpc/codes.go:55: func StatusCode(status int) codes.Code {
Are all of these codes necessary? IMO, this should map only codes that can be
emitted before the GRPC header can be added. Since you add it 100% of the time
your pRPC server code gets control, this turns into the set of codes returned by
the web server before it calls pRPC, namely:

401, 403, 404, 500+

Is there a reason to map any other codes?

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/helloworld...
File common/prpc/helloworld_test.pb_test.go (right):

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/helloworld...
common/prpc/helloworld_test.pb_test.go:31: func (m *HelloRequest) Reset()       
            { *m = HelloRequest{} }
This looks like it dropped tabs?

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/options.go...
common/prpc/options.go:20: UserAgent string                // User-Agent HTTP
header. If empty, DefaultUserAgent is used.
Is this >80 characters? Maybe comment on separate line.

nodir

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go File common/prpc/client.go (right): https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#newcode68 common/prpc/client.go:68: fields := logging.Fields{ On 2016/01/22 01:16:27, dnj wrote: > ...

4 years, 11 months ago (2016-01-22 02:57:50 UTC) #11

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:68: fields := logging.Fields{
On 2016/01/22 01:16:27, dnj wrote:
> On 2016/01/22 00:47:23, nodir wrote:
> > On 2016/01/21 07:44:28, dnj wrote:
> > > Fields layer naturally via "SetFields". ctx = logging.SetFields(ctx,
> > > logging.Fields{...}), then you basically get this for free with a lot less
> > code.
> > > All future calls logging.Errorf(ctx, "ohaithere") will inherit the
installed
> > > fields.
> > > 
> > > Granted it costs a Context derivation but ... who cares?
> > 
> > ah, I see. Done, but I don't see much  code-size savings. Am I using fields
> > right?
> 
> Looks like you are.
> 
> It's less about code size changes and more reliving the burden of management
> from you. You can now assume these fields will always be there and move on
with
> your life.

ok so there is no "a lot less code"

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:91: return errors.WrapTransient(fmt.Errorf("failed to send
request: %s", err))
On 2016/01/22 01:16:27, dnj wrote:
> On 2016/01/22 00:47:24, nodir wrote:
> > On 2016/01/21 07:44:28, dnj wrote:
> > > IMO you should handle transient errors more surgically. Maybe >= 500?
> > 
> > status is not available if err != nil
> 
> This sounds like a hard failure to me then, not a transient one.

Client.Do may return an error on URLFetch proxy failure
(https://godoc.org/google.golang.org/appengine/urlfetch#Transport.RoundTrip).
Without retries RPCs might be unreliable

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:100: if res.StatusCode >= 300 {
On 2016/01/22 01:16:27, dnj wrote:
> On 2016/01/22 00:47:24, nodir wrote:
> > On 2016/01/21 07:44:27, dnj wrote:
> > > Maybe http.StatusMultipleChoices instead of 300?
> > 
> > name "MultipleChoises" does not imply it is the first error status code. We
> > could have "firstBadStatus" constant (see also these markers in
> > https://golang.org/src/go/token/token.go#L22), but I believe HTTP status
codes
> > are uniquos enough to know that bad statuses start with 300
> 
> Actually now that I think about it, you control the server, so you should
> whitelist specific success codes. Maybe if StatusCode != OK? OK, NoContent?

Why couple tightly? HTTP status codes < 300 are not errors

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:107: return errors.WrapTransient(fmt.Errorf("failed to
read response body: %s", err))
On 2016/01/22 01:16:27, dnj wrote:
> On 2016/01/22 00:47:24, nodir wrote:
> > On 2016/01/21 07:44:27, dnj wrote:
> > > Why is a failure to read body transient? This would only happen if TCP is
> > > interrupted 
> > 
> > Isn't TCP interruption transient? In other words, if we repeat the request
we
> > may get lucky.
> 
> It depends why the interruption happened. e.g, if there was a data error in
the
> HTTP response, it might not be transient. Regardless, the TCP/IP protocol will
> take care of any transient cases of this (data corruption, interruption,
etc.).
> I don't think this is an application layer transient failure case.
> 
> > > or if there is an error in the content decoding, both of which seem
> > > like non-transient.
> > 
> > What decoding you are talking about? Isn't this raw bytes?
> 
> res.Body's reader wraps GZIP and chunk decoding from the raw HTTP response. It
> will return the raw bytes on success, but can fail if either of those
decodings
> don't work.

Done

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:33: HeaderGrpcCode = "X-Prpc-Grpc-Code"
On 2016/01/22 01:16:27, dnj wrote:
> nit: HeaderGRPCCode?

Done.

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:120: bodySize := c.ErrBodySize
On 2016/01/22 01:16:27, dnj wrote:
> Move this logic into responseErr and make responseErr a Client member
function?

Done.

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:131: buf.Grow(int(req.ContentLength))
On 2016/01/22 01:16:27, dnj wrote:
> <res>.ContentLength** (and above)

oops done

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:134: if _, err = io.Copy(&buf, body); err != nil {
On 2016/01/22 01:16:27, dnj wrote:
> io.Copy reads in fixed-size chunks. You should use buf.ReadFrom, which reads
in
> buf-sized chunks if available. This would result in potentially zero
> reallocations if the ContentLength was supplied and better performance in
retry
> cases.

Done.

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:149: }.Warningf(ctx, "RPC failed transiently. Will retry
in %s", sleepTime)
On 2016/01/22 01:16:27, dnj wrote:
> nit: Since you're emitting sleepTime as a Field, you don't need to repeat it
in
> the error string.

I don't like how fields are printed and consider them auxilliary to the log
message. My position is to have a nice log message first, and secondly extra
data in the fields. In other words, the fact that something is present in the
fields should be the reason to exclude it from the log message.

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/codes.go#n...
common/prpc/codes.go:55: func StatusCode(status int) codes.Code {
On 2016/01/22 01:16:27, dnj wrote:
> Are all of these codes necessary? IMO, this should map only codes that can be
> emitted before the GRPC header can be added. Since you add it 100% of the time
> your pRPC server code gets control, this turns into the set of codes returned
by
> the web server before it calls pRPC, namely:
> 
> 401, 403, 404, 500+
> 
> Is there a reason to map any other codes?

I've added the header in this CL for cases when a pRPC server received a gRPC
code from a service, converted it to an ambiguous HTTP status and we want
exactly the same gRPC code on the client. But there are cases when there is no
original gRPC code. I've updated the server code to include the header only if
the server knew the original gRPC code, rather than derived it somehow.

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/helloworld...
File common/prpc/helloworld_test.pb_test.go (right):

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/helloworld...
common/prpc/helloworld_test.pb_test.go:31: func (m *HelloRequest) Reset()       
            { *m = HelloRequest{} }
On 2016/01/22 01:16:27, dnj wrote:
> This looks like it dropped tabs?

Dunno, it is currently well-formatted

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/options.go...
common/prpc/options.go:20: UserAgent string                // User-Agent HTTP
header. If empty, DefaultUserAgent is used.
On 2016/01/22 01:16:27, dnj wrote:
> Is this >80 characters? Maybe comment on separate line.

Done.

dnj (Google)

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go File common/prpc/client.go (right): https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#newcode68 common/prpc/client.go:68: fields := logging.Fields{ On 2016/01/22 02:57:50, nodir wrote: > ...

4 years, 11 months ago (2016-01-22 04:06:24 UTC) #13

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

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:68: fields := logging.Fields{
On 2016/01/22 02:57:50, nodir wrote:
> On 2016/01/22 01:16:27, dnj wrote:
> > On 2016/01/22 00:47:23, nodir wrote:
> > > On 2016/01/21 07:44:28, dnj wrote:
> > > > Fields layer naturally via "SetFields". ctx = logging.SetFields(ctx,
> > > > logging.Fields{...}), then you basically get this for free with a lot
less
> > > code.
> > > > All future calls logging.Errorf(ctx, "ohaithere") will inherit the
> installed
> > > > fields.
> > > > 
> > > > Granted it costs a Context derivation but ... who cares?
> > > 
> > > ah, I see. Done, but I don't see much  code-size savings. Am I using
fields
> > > right?
> > 
> > Looks like you are.
> > 
> > It's less about code size changes and more reliving the burden of management
> > from you. You can now assume these fields will always be there and move on
> with
> > your life.
> 
> ok so there is no "a lot less code"

Well, depends, each logging message where you don't have to copy/append and
remember fields is less code. In LogDog, I set fields up front, so that would be
a lot less. Maybe I should have said, "a lot less boilerplate?"

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:91: return errors.WrapTransient(fmt.Errorf("failed to send
request: %s", err))
On 2016/01/22 02:57:50, nodir wrote:
> On 2016/01/22 01:16:27, dnj wrote:
> > On 2016/01/22 00:47:24, nodir wrote:
> > > On 2016/01/21 07:44:28, dnj wrote:
> > > > IMO you should handle transient errors more surgically. Maybe >= 500?
> > > 
> > > status is not available if err != nil
> > 
> > This sounds like a hard failure to me then, not a transient one.
> 
> Client.Do may return an error on URLFetch proxy failure
> (https://godoc.org/google.golang.org/appengine/urlfetch#Transport.RoundTrip).
> Without retries RPCs might be unreliable

Ugh okay. I really dislike that they don't give us a means to distinguish
between a flake (proxy failure) and an actually error in our request
(nonexistentdomainname.com doesn't exist) :/ Better to error on the side of
flake, I suppose.

https://codereview.chromium.org/1605363002/diff/80001/common/prpc/client.go#n...
common/prpc/client.go:100: if res.StatusCode >= 300 {
On 2016/01/22 02:57:50, nodir wrote:
> On 2016/01/22 01:16:27, dnj wrote:
> > On 2016/01/22 00:47:24, nodir wrote:
> > > On 2016/01/21 07:44:27, dnj wrote:
> > > > Maybe http.StatusMultipleChoices instead of 300?
> > > 
> > > name "MultipleChoises" does not imply it is the first error status code.
We
> > > could have "firstBadStatus" constant (see also these markers in
> > > https://golang.org/src/go/token/token.go#L22), but I believe HTTP status
> codes
> > > are uniquos enough to know that bad statuses start with 300
> > 
> > Actually now that I think about it, you control the server, so you should
> > whitelist specific success codes. Maybe if StatusCode != OK? OK, NoContent?
> 
> Why couple tightly? HTTP status codes < 300 are not errors

If we fully control our server's response codes and only use OK, and we get
"Partial Content", "Reset Content", "IM Used", "Continue", "Switching
Protocols", or any other <300 response codes, something is wrong, aka error.

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/client.go#...
common/prpc/client.go:149: }.Warningf(ctx, "RPC failed transiently. Will retry
in %s", sleepTime)
On 2016/01/22 02:57:50, nodir wrote:
> On 2016/01/22 01:16:27, dnj wrote:
> > nit: Since you're emitting sleepTime as a Field, you don't need to repeat it
> in
> > the error string.
> 
> I don't like how fields are printed and consider them auxilliary to the log
> message. My position is to have a nice log message first, and secondly extra
> data in the fields. In other words, the fact that something is present in the
> fields should be the reason to exclude it from the log message.

The purpose of "fields" is to remove useful data from the log message string so
that it's concentrated and (potentially) exportable. They shouldn't be
considered auxiliary, just slightly more structured than a raw string. But up to
you.

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

https://codereview.chromium.org/1605363002/diff/100001/common/prpc/codes.go#n...
common/prpc/codes.go:55: func StatusCode(status int) codes.Code {
On 2016/01/22 02:57:50, nodir wrote:
> On 2016/01/22 01:16:27, dnj wrote:
> > Are all of these codes necessary? IMO, this should map only codes that can
be
> > emitted before the GRPC header can be added. Since you add it 100% of the
time
> > your pRPC server code gets control, this turns into the set of codes
returned
> by
> > the web server before it calls pRPC, namely:
> > 
> > 401, 403, 404, 500+
> > 
> > Is there a reason to map any other codes?
> 
> I've added the header in this CL for cases when a pRPC server received a gRPC
> code from a service, converted it to an ambiguous HTTP status and we want
> exactly the same gRPC code on the client. But there are cases when there is no
> original gRPC code. I've updated the server code to include the header only if
> the server knew the original gRPC code, rather than derived it somehow.

Wait, does the server actually respond with both gRPC code header _and_
translated status? I think that's a bad idea. HTTP status is understood by
intermediate agents, which may take some action or other (caching, auto-backoff,
etc.). I don't think we want this with RPC calls. HTTP is a necessary evil
medium, not a desired protocol to bootstrap off of.

The way I see it, there are two worlds:
1) Our request didn't make it to our pRPC server. In this case, the
infrastructure will return some code.
2) Our request made it to a pRPC server, in which case we should always return
OK w/ a gRPC error header if there was an error. Similar to http.RoundTripper.

In this case, I don't think we should map any codes other than those we expect
from (1), and those should all map to "Unknown".

What's the use case for duplicating the gRPC response in the HTTP header? Our
code (the only thing that should be talking to pRPC server) is going to use the
header. Any other services shouldn't be calling pRPC.

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

https://codereview.chromium.org/1605363002/diff/120001/common/prpc/options.go...
common/prpc/options.go:19: Retry func() retry.Iterator // RPC retrial.
Just uploaded a CL that changed how Retry works. Now this is called a
"retry.Factory" and is first-class passable to Retry!

dnj

4 years, 11 months ago (2016-01-27 00:14:11 UTC) #14

Continuing here:
https://codereview.chromium.org/1637193002

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages