Skip to content
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

Migrate service proxy and interceptor from SDK #100

Merged
merged 27 commits into from
Jan 5, 2023
Merged

Conversation

robholland
Copy link
Contributor

@robholland robholland commented Dec 2, 2022

What changed?

Ported gRPC interceptor and proxy code from sdk-go.

Why?

This allows us to keep the proxy and interceptor inline with the API version and allows varying the API version used by SDKs. Previously the large amount of types touched by the interceptor would cause incompatibility when the API changed.

temporalio/sdk-go#971

How did you test it?

Added some tests and adapter sdk-go to use the API code, also with tests.

Potential risks

This should be a no-op, a bug may cause gRPC proxy or interceptors using these APIs to misbehave.

cmd/generateproxy/interceptor.go Outdated Show resolved Hide resolved
proxy/interceptor.go Outdated Show resolved Hide resolved
proxy/interceptor.go Outdated Show resolved Hide resolved
proxy/interceptor.go Outdated Show resolved Hide resolved
proxy/interceptor.go Outdated Show resolved Hide resolved
@robholland robholland changed the title Initial version. Migrate service proxy and interceptor from SDK Dec 2, 2022
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Lets get a test or two in here and if you don't mind a GH action to run on each PR just in case (w/ test and lint). I can help here if needed.

cmd/generateproxy/interceptor.go Outdated Show resolved Hide resolved
@cretz
Copy link
Member

cretz commented Dec 5, 2022

Can we also add at least one Go test to the package and run go test on the whole project? This is a sanity check to ensure compilation among other things.

@robholland
Copy link
Contributor Author

Yep I'll copy/amend the SDK unit tests over this afternoon.

@robholland robholland marked this pull request as ready for review December 5, 2022 20:25
@robholland robholland requested review from a team as code owners December 5, 2022 20:25
@robholland robholland requested a review from cretz December 5, 2022 20:43
@cretz
Copy link
Member

cretz commented Dec 6, 2022

@robholland - we have another use case from temporalio/sdk-go#930. They want to visit every temporal.api.failure.v1.Failure in the proxy. I know it's kinda annoying to ask at last minute, but think we can genericize this a bit? I am wondering if now is the time to do this.

Maybe change VisitPayloads to VisitProtos (I understand we limit to only where payloads are today, is it that troubling to change to walk all protos?). We could still have separate "payload visitor" and "failure visitor" though having some kind of common proto visitor makes more sense, even if it's just interface{} because sometimes it's a slice instead of a proto.Message.

Any design ideas on how best to incorporate failure visiting into the proxy? I have some more detailed thoughts.

@bergundy
Copy link
Member

bergundy commented Dec 6, 2022

@robholland - we have another use case from temporalio/sdk-go#930. They want to visit every temporal.api.failure.v1.Failure in the proxy. I know it's kinda annoying to ask at last minute, but think we can genericize this a bit? I am wondering if now is the time to do this.

Let’s do this please while we’re touching all of this and avoid the risk of more compilation breaking changes for the Go SDK.

Add some unit tests.
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would like @alexshtin and/or @yiminc to take a look

proxy/interceptor.go Outdated Show resolved Hide resolved
@cretz
Copy link
Member

cretz commented Dec 29, 2022

@robholland - we just noticed a bug in the Go SDK version of this where the map[string]*commonpb.Payloads (note the plural) isn't in the switch statement. Can you add it here?

@robholland
Copy link
Contributor Author

Added.

Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nit style comments. Didn't go much in generator code, but generated code looks good. No human should review reflection and reflection generated code though.

cmd/generateproxy/main.go Outdated Show resolved Hide resolved
cmd/generateproxy/go.mod Outdated Show resolved Hide resolved
cmd/generateproxy/service.go Outdated Show resolved Hide resolved
cmd/generateproxy/interceptor.go Outdated Show resolved Hide resolved
cmd/generateproxy/service.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants