Improve proxy codegen by using proto descriptors #210
Conversation
0ae9bbd to
452e2e7
Compare
| case *errordetails.MultiOperationExecutionFailure: | ||
|
|
There was a problem hiding this comment.
Some things did, in fact, get missed!
There was a problem hiding this comment.
I am not sure if the proxy ever look into error details though...
There was a problem hiding this comment.
What do you mean? It won't call with them? If so, oh well, no harm having it here anyway since they do contain payloads
There was a problem hiding this comment.
Yeah because these are part of the error
It won't call with them? If so, oh well
Well the consequence is a payload or failure that should be encoded would not be so it is kinda important no?
There was a problem hiding this comment.
Oh, I see what you mean now.
Well that's because it's not referenced by any other proto. In fact I don't understand this at all:
// ExecuteMultiOperation executes multiple operations within a single workflow.
//
// Operations are started atomically, meaning if *any* operation fails to be started, none are,
// and the request fails. Upon start, the API returns only when *all* operations have a response.
//
// Upon failure, it returns `MultiOperationExecutionFailure` where the status code
// equals the status code of the *first* operation that failed to be started.
//
// NOTE: Experimental API.
rpc ExecuteMultiOperation (ExecuteMultiOperationRequest) returns (ExecuteMultiOperationResponse) {
That comment is the only place it's referenced
There was a problem hiding this comment.
I think it is part of the error details https://grpc.io/docs/guides/error/#richer-error-model
There was a problem hiding this comment.
The proxy doesn't look at errors at all right now AFAIK @cretz please correct me if I am wrong
There was a problem hiding this comment.
Not saying you need to fix this as part of this PR, but it is another gap I think
There was a problem hiding this comment.
Oh, I see, OK that makes sense. Yeah, I agree that is a possible gap. I also think probably another PR.
There was a problem hiding this comment.
The proxy doesn't look at errors at all right now AFAIK @cretz please correct me if I am wrong
Correct, ref
Lines 99 to 101 in dad8b16
ExecuteMultiOperation and QueryWorkflow. I agree it is a hole in our logic in the interceptor that we should fix (walk the gRPC status details). I have opened temporalio/sdk-go#1825. But yes we should keep them in this switch statement of course.
proxy/interceptor.go
Outdated
| if err := visitFailures( | ||
| ctx, | ||
| options, | ||
| o.GetUserMetadata(), |
There was a problem hiding this comment.
GetUserMetadata doesn't contain a failure so why would we need to search it?
There was a problem hiding this comment.
It looks like maybe some of these are still reporting as being needed to search because they have payloads rather than failures, I'll see if that's it.
There was a problem hiding this comment.
Ok these were fixed by removing the hardcoded check for payloads in walk
|
I changed the make stuff to generate the proto descriptors on the fly since checking in a 1mb file is annoying. However that would mean adding proto to CI which isn't there currently. Should we just add it? Discussed @ SDK standup. No objections to adding protoc to CI |
441650a to
062634e
Compare
| case *errordetails.MultiOperationExecutionFailure: | ||
|
|
There was a problem hiding this comment.
The proxy doesn't look at errors at all right now AFAIK @cretz please correct me if I am wrong
Correct, ref
Lines 99 to 101 in dad8b16
ExecuteMultiOperation and QueryWorkflow. I agree it is a hole in our logic in the interceptor that we should fix (walk the gRPC status details). I have opened temporalio/sdk-go#1825. But yes we should keep them in this switch statement of course.
There was a problem hiding this comment.
Hrmm, I didn't dig, but why are these files appearing now?
There was a problem hiding this comment.
I needed to regenerate these to work properly with the updates to the proto lib
There was a problem hiding this comment.
Hrmm, ok. I forget, but I guess as part of the the tests we added the generated proto code in we never committed the protos themselves?
| if resultType.String() == types.NewPointer(directMatchType).String() { | ||
| hasDirectMatch = true |
There was a problem hiding this comment.
Don't love that I had to do this but I could not for the life of me get reflection to be equal properly
Closes temporalio/sdk-go#1811
What changed?
Title. More reliable way of discovering everything
Why?
Avoid missing newly added top-level paths to payloads/failures
How did you test it?
Existing tests / comparing generated output.
Potential risks
Too much visiting!