-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gRPC transport initial work with crossdock testing #863
Changes from 55 commits
fea2579
8ccec60
840241a
c0c9838
f20daac
4af2e29
9cb61f4
87d286f
0c58468
2613453
8fdfd51
328a2cb
82a0b02
48c4329
13b13f2
77acfec
904c1f2
30740b3
47cefc5
d3937f3
54735d0
d1803f7
3e38dfe
45bf89c
bec7415
1589afa
2b77918
07f2c92
17d99d5
d763d72
cc8c00a
c96fc1d
e43151e
99c0e26
f9ac97b
ec85413
1ebc702
87a4135
0ce6d0d
7fb63ef
9e8fc53
1dec7c5
d223951
ab09c97
ae2a78c
92c7315
2a66063
45f341a
a8de846
e3de61f
fae985f
1e2ac25
0808d2c
8a59b68
dd7f754
5466aae
01c1c08
d12b770
b88fc2f
552ffdf
b820f6d
0179178
c82f69f
70e38bb
87cf60e
293b14a
6ceae8a
daef54e
922da87
3ad0cef
bcec6ce
1e4d169
f1c57aa
32ed9ce
48a227d
c9d1e73
d6d663d
f908f8b
9f56edf
7ec7ed1
8242208
d608ee7
51c4fb4
65d9cff
d61fd0f
70b259e
b159f50
1c500d3
ac5cd33
9e8d197
210af50
3d9f351
fb9f0e6
5529d7a
e0b13f4
2cc2444
725b8cb
c3d649e
9980462
00d8552
f3ffdef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
# Paths besides auto-detected generated files that should be excluded from | ||
# lint results. | ||
LINT_EXCLUDES_EXTRAS = | ||
LINT_EXCLUDES_EXTRAS = \ | ||
transport/x/grpc/handlers.go | ||
|
||
# Regex for 'go vet' rules to ignore | ||
GOVET_IGNORE_RULES = \ | ||
|
@@ -279,7 +280,7 @@ goveralls: $(BIN)/goveralls | |
|
||
.PHONY: examples | ||
examples: | ||
RUN=$(RUN) $(MAKE) -C internal/examples | ||
RUN=$(RUN) V=$(V) $(MAKE) -C internal/examples | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. verbose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is consistent with UberFX heh. |
||
|
||
.PHONY: crossdock | ||
crossdock: $(DOCKER_COMPOSE) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,9 @@ func (u *unaryHandler) Handle(ctx context.Context, transportRequest *transport.R | |
} | ||
} | ||
response, appErr := u.handle(ctx, request) | ||
if appErr != nil { | ||
responseWriter.SetApplicationError() | ||
} | ||
if err := call.WriteToResponse(responseWriter); err != nil { | ||
return err | ||
} | ||
|
@@ -78,9 +81,16 @@ func (u *unaryHandler) Handle(ctx context.Context, transportRequest *transport.R | |
} | ||
responseData = protoBuffer.Bytes() | ||
} | ||
if isRawResponse(transportRequest.Headers) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a comment explaining how this is for the decision whether we want to envelope the response or not (if that's what it is for, I'll admit I'm not 100% sure) Would also love a task/issue that explains the issue in detail so we can have some back and forth convo on what's going on here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only way to accomplish this is to do this on the headers, believe me I tried other methods, unsuccessfully. I think we can start discussing this on #905 but we may want another issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a comment explaining this |
||
responseWriter.AddHeaders(getRawResponseHeaders()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, staring at this I'm still a bit confused, are we talking between the encoding and transport via headers on the request/response? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment on #905 and the linked commit - this is the only way to accomplish this two-way communication with the protobuf encoding, it is not possible to do this with the |
||
_, err := responseWriter.Write(responseData) | ||
if err != nil { | ||
return err | ||
} | ||
return appErr | ||
} | ||
var wireError *wirepb.Error | ||
if appErr != nil { | ||
responseWriter.SetApplicationError() | ||
wireError = &wirepb.Error{ | ||
appErr.Error(), | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,17 +31,20 @@ import ( | |
"github.com/gogo/protobuf/proto" | ||
) | ||
|
||
// Encoding is the name of this encoding. | ||
const Encoding transport.Encoding = "protobuf" | ||
const ( | ||
// Encoding is the name of this encoding. | ||
Encoding transport.Encoding = "protobuf" | ||
|
||
// GetApplicationError returns the application error from the server, if present. | ||
rawResponseHeaderKey = "yarpc-protobuf-raw-response" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming that we don't need to express application errors in the response body for Protobuf. I’m proposing an alternative to this header #905 Whether the body is present can be implied by an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a larger discussion we need to have before moving gRPC out of x/ as to how transports will understand application errors, as not all encodings are able to specify application errors in the same manner as thrift, leaving this for now. |
||
) | ||
|
||
// SetRawResponse will set rawResponseHeaderKey to "true". | ||
// | ||
// TODO: this has overlap with IsApplicationError | ||
func GetApplicationError(err error) error { | ||
if applicationError, ok := err.(*applicationError); ok { | ||
return applicationError | ||
} | ||
return nil | ||
// rawResponseHeaderKey is a header key attached to either a request or | ||
// response that signals a UnaryHandler to not encode an application error | ||
// inside a wirepb.Response object, instead marshalling the actual response. | ||
func SetRawResponse(headers transport.Headers) transport.Headers { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure headers should be used to share information in the process? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #905, and the corresponding comment I added on |
||
return headers.With(rawResponseHeaderKey, "1") | ||
} | ||
|
||
// ***all below functions should only be called by generated code*** | ||
|
@@ -118,3 +121,12 @@ func NewOnewayHandler( | |
func CastError(expectedType proto.Message, actualType proto.Message) error { | ||
return fmt.Errorf("expected proto.Message to have type %T but had type %T", expectedType, actualType) | ||
} | ||
|
||
func isRawResponse(headers transport.Headers) bool { | ||
rawResponse, ok := headers.Get(rawResponseHeaderKey) | ||
return ok && rawResponse == "1" | ||
} | ||
|
||
func getRawResponseHeaders() transport.Headers { | ||
return SetRawResponse(transport.Headers{}) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,10 @@ import: | |
version: ~0.4 | ||
- package: github.com/golang/mock | ||
version: master | ||
- package: github.com/grpc-ecosystem/grpc-opentracing | ||
version: master | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No semver here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. grpc-opentracing isn't doing releases unfortunately :( we need to evaluate grpc-opentracing more anyways before grpc can get out of the x/ package |
||
subpackages: | ||
- go/otgrpc | ||
- package: github.com/mattn/go-shellwords | ||
version: ^1 | ||
- package: github.com/mitchellh/mapstructure | ||
|
@@ -43,6 +47,9 @@ import: | |
repo: https://github.com/golang/net | ||
subpackages: | ||
- context | ||
- package: google.golang.org/grpc | ||
version: ^1.2 | ||
repo: https://github.com/grpc/grpc-go | ||
- package: golang.org/x/sys | ||
# explicitly specifying this because glide is having issues with golang.org repos | ||
# this is just a dependency of golang.org/x/net and if this problem is fixed | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we keep the comment about how the grpc interface doesn't meet lint rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I don't know, I think this is fine for now