-
Notifications
You must be signed in to change notification settings - Fork 215
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
Move namespace of proto generated C++ types #4323
Conversation
grpc_ccf@51029 aka 20221007.18 vs main ewma over 20 builds from 50620 to 51025 Click to see tablemain
grpc_ccf
|
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.
I wonder if we should instead rename the externalexecutor
to remotedispatcher
(or executordispatcher
) in this PR or make a separate one for it. I think it would be good to have this reflect it, so that we comply with it in future code changes.
I vote for a separate PR, though yes this is another place where it should be renamed! |
Resolves #4312.
I opted for a
protobuf
namespace rather than agrpc
namespace, since we're purely talking about the generated protobuf types (we don't use the generated gRPC stubs). Also distinguishedccf::protobuf
(things that might be shared as part of general CCF gRPC support) andexternalexecutor::protobuf
(things extremely specific to theexternal_executor
execution engine).The annoying side effect of this is that we also change the URLs where these RPCs are served, but I think that's fine because they're hidden in generated gRPC code. I thought we could override those with the
google.api.http
option, but it turns out that's purely about the JSON mapping, not the gRPC-over-HTTP2 mapping.