-
Notifications
You must be signed in to change notification settings - Fork 6
Add metadata for gRPC reflection API #50
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
base: main
Are you sure you want to change the base?
Conversation
andersfugmann
left a comment
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.
Thanks. LGTM. I've have one question on naming though.
Also before we merge, could you give a simple example of all the metadata created around service endpoints for gRPC? I'd like to understand if more fields could be moved into this structure to clean up and hopefully it would also be easier to add additional fields going forward.
src/ocaml_protoc_plugin/spec.ml
Outdated
| | Bytes -> Bytes.create 0 | ||
| | Enum (module Enum) -> Enum.from_int_exn 0 | ||
|
|
||
| module type Metainfo = sig |
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.
Does metainfo have a special definition in the gRPC standard? If not, then I think it would be better to call it something like Service_info
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.
No it may not be a standard. I agree with Service_info 👍
|
@andersfugmann https://github.com/Nymphium/oresai/blob/main/server/protos/generated/grpc_reflection_v1_reflection.ml#L2025 |
|
Thanks for providing examples. Would it be possible to make the list of endpoints be a list of modules instead to avoid string duplication? let package_service_names =
[ (module Grpc.Reflection.V1.ServerReflection : Rpc); ... ]Should hold the same (if not more) information and avoid string duplication. Btw. Since this information is only needed for gRPC, maybe call the module |
|
@andersfugmann I've applied the changes in 4fb8d6e.
I addeed
It's difficult to achieve that in some cases. |
andersfugmann
left a comment
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.
Looks very good. Thanks for making the updates.
| (* Code.emit implementation `None "%s" (Code.append_deprecaton_if ~deprecated `Floating ""); *) | ||
| () | ||
|
|
||
| let emit_metainfo implementation fd file_name package_service_names = |
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.
This should be called emit_service_info
| | Bytes -> Bytes.create 0 | ||
| | Enum (module Enum) -> Enum.from_int_exn 0 | ||
|
|
||
| module type Service_info = sig |
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.
The declaration of the Service_info should be moved to the Service module.
I'm not sure I follow. The service names are already collected as part of emitting the signature and implementation of all messages and services defined in the proto file. I may take a stab at implementing that, but I would prefer to the two other comments + tests addressed first. Again, Thanks for doing this work. Really appreciated! |
This PR adds
Metainfomodule to each generated files.That module has information to implement gRPC reflection API: