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

GRPC Service Proxy and Encoding Interceptor #738

Merged
merged 38 commits into from
Mar 2, 2022

Conversation

robholland
Copy link
Contributor

@robholland robholland commented Feb 24, 2022

What was changed

Added a proxy which wires together a GRPC server and client for WorkflowService.

Why?

This allows users to build proxies for Temporal Frontend traffic and use GRPC interceptors to manipulate requests or responses. One use case for this is to centralise data encoding.

Checklist

  1. How was this tested:

  2. Any docs updates needed?

@robholland robholland changed the title Rh encoding interceptor GRPC Service Proxy and Encoding Interceptor Feb 25, 2022
@robholland robholland marked this pull request as ready for review February 26, 2022 00:18
@robholland
Copy link
Contributor Author

I've got an auto-generated version of the interceptor nearly working which would use an opt-out approach to exclude Payload(s) we don't want to be touched. Chad has suggested a test which will re-walk the API definitions and check if the list on which the interceptor is built has drifted from the current API version so that we can ensure decisions are made about new Payload(s) as they are introduced (or removed for that matter).

@bergundy
Copy link
Member

Which payloads do we not want to touch?

@robholland
Copy link
Contributor Author

SearchAttributes is the only one I'm aware of.

@cretz
Copy link
Member

cretz commented Feb 28, 2022

When temporalio/api#153 lands, it'll be a good example of whether the test will fail when we upgrade the API dependency but don't re-gen code.

@bergundy
Copy link
Member

I've got an auto-generated version of the interceptor nearly working which would use an opt-out approach to exclude Payload(s) we don't want to be touched. Chad has suggested a test which will re-walk the API definitions and check if the list on which the interceptor is built has drifted from the current API version so that we can ensure decisions are made about new Payload(s) as they are introduced (or removed for that matter).

Nice! That's what I was going for

@robholland
Copy link
Contributor Author

make check now verifies via make generatorcheck that files generated based on the API are not stale.

@robholland robholland requested review from bergundy and cretz March 1, 2022 16:23
@cretz
Copy link
Member

cretz commented Mar 1, 2022

LGTM! Ignore the coveralls complaint (can squash/merge anyways once you get @bergundy's approval)

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Just approved.
I didn't go over the code again but I understand the new approach which is the right way to go IMO.

@robholland robholland merged commit 2c5ed0f into master Mar 2, 2022
@robholland robholland deleted the rh-encoding-interceptor branch March 2, 2022 08:36
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.

3 participants