-
Notifications
You must be signed in to change notification settings - Fork 444
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
generated-code: update to include stream generics #1365
base: main
Are you sure you want to change the base?
Conversation
Cannot add reviewer, @purnesh42H please review |
@ejona86 can you take a look at this? |
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.
lgtm
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.
Might be good to cross check here if we need to edit/add anything as we recently updated this https://github.com/grpc/grpc-go/blob/00b9e140ce71480ee7ecc6b85317021e0fe11fbb/stream_interfaces.go#L21
messages from the client. `Recv` returns `(nil, io.EOF)` once it has reached the end of the stream. | ||
The single response message from the server is sent by calling the `SendAndClose` method on this `<ServiceName>_FooServer` parameter. | ||
Note that `SendAndClose` must be called once and only once. | ||
The `grpc.ClientStreamingServer` has two methods `Recv()` and `SendAndClose(*MsgB)` , has an embedded `ServerStream`. The server-side handler can repeatedly call `Recv` on `<ServiceName>_FooServer` in order to receive the full stream of messages from the client. `Recv` returns `(nil, io.EOF)` once it has reached the end of the stream. The single response message from the server is sent by calling the `SendAndClose` method on this `<ServiceName>_FooServer` parameter. Note that `SendAndClose` must be called once and only once. |
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.
SendAndClose(*MsgB)
, -> SendAndClose(*MsgB)
,
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.
Done
|
||
In this context, `<ServiceName>_FooClient` represents the server-to-client `stream` of `MsgB` messages. | ||
In this context, `grpc.ServerStreamClient` represents the client side of server-to-client stream of `MsgB` messages. |
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 like there is extra space after grpc.ServerStreamClient
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.
Done
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.
Please apply the comments throughout, and please review your changes for correctness in light of all the typos. Thanks!
|
||
`Foo(*MsgA, <ServiceName>_FooServer) error` | ||
In this context, `MsgA` is the single request from the client, and , and `grpc.ServerStreamingServer` represents the server side of server-to-client stream of response type `MsgB`. |
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.
Also an extra space in the server side
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.
Done
of `MsgB` messages. | ||
|
||
`<ServiceName>_FooServer` has an embedded `grpc.ServerStream` and the following interface: | ||
`<ServiceName>_FooServer` has a `grpc.SreverStreamingServer` interface that takes `MsgB` i.e. the response type as the input: |
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.
Srever -> Server
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.
Done
of `MsgB` messages. | ||
|
||
`<ServiceName>_FooServer` has an embedded `grpc.ServerStream` and the following interface: | ||
`<ServiceName>_FooServer` has a `grpc.SreverStreamingServer` interface that takes `MsgB` i.e. the response type as the input: |
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.
Grammar: please use commas:
MsgB
, i.e. the response type, as the input.
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.
Done
``` | ||
|
||
The server-side handler can send a stream of protobuf messages to the client through this parameter's `Send` method. End-of-stream for the server-to-client | ||
stream is caused by the `return` of the handler method. | ||
The `grpc.SreverStreamingServer` has a `Send(*MsgB)` method and has `ServerStream` embedded in it. |
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.
Typo.
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.
Done
Send(*MsgB) error | ||
grpc.ServerStream | ||
} | ||
type <ServiceName>_FooServer = grpc.ServerStreamingServer[*MsgB] |
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.
Let's put documentation for the ServerStreamingServer
here instead and leave out this leftover type alias, which is only there for backward compatibility. We can probably rely heavily on the existing documentation, or maybe even just link it: https://pkg.go.dev/google.golang.org/grpc#ServerStreamingServer
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.
Delete this?
|
||
`<ServiceName>_FooClient` has an embedded `grpc.ClientStream` and the following interface: | ||
`<ServiceName>_FooServe`r has a `grpc.ClientStreamingClient` interface that takes `MsgA` and `MsgB` as input: |
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.
Typo
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.
Done.
Send(*MsgB) error | ||
grpc.ServerStream | ||
} | ||
type <ServiceName>_FooServer = grpc.ServerStreamingServer[*MsgB] |
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.
Delete this?
of `MsgB` messages. | ||
|
||
`<ServiceName>_FooServer` has an embedded `grpc.ServerStream` and the following interface: | ||
`<ServiceName>_FooServer` has a `grpc.ServerStreamingServer` interface that takes `MsgB`, i.e. the response type, as the input. |
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 think we shouldn't mention <ServiceName>_FooServer
, since that name is no longer part of the generated code, except for backward compatibility.
But I wonder if we should have two documents, and link between them: one for the old generated code and one for the new generated code. So we can keep the old code for people that are working with an old generated file.
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.
Done.
This describes the code generated with the [grpc plugin](https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc), `protoc-gen-go-grpc`, | ||
when compiling `.proto` files with `protoc` without the generics. | ||
|
||
|
||
## Methods on generated server interfaces |
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.
Can we move the old documentation into another page and link to it from the top of this page? Just a little note like "The following documents the latest generated code guide which uses generics. For documentation about the older version of generated code that did not use generics, please see <this document>."
?
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.
Done.
@@ -34,68 +35,29 @@ In this context, `MsgA` is the protobuf message sent from the client, and `MsgB` | |||
### Server-streaming methods | |||
|
|||
These methods have the following signature on the generated service interface: | |||
`Foo(*MsgA, grpc.ServerStreamingServer[*MsgB]) error` |
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 think it would help to rename these from MsgA and MsgB to RequestMsg and ResponseMsg. WDYT?
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.
Sounds good to me.
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.
Changed in the new doc, did not change in the documentation for old generated code for continuity of users who might have been referring to it. Or should it be changed in the old documentation too.
} | ||
``` | ||
|
||
The server-side handler can send a stream of protobuf messages to the client through this parameter's `Send` method. End-of-stream for the server-to-client |
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.
Since all this documentation was deleted, let's explicitly call out that instructions on how to use the Stream type can be found at the pkg.go.dev link above.
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.
Done
The following documents the latest generated code guide which uses generics. For documentation about the older version of generated code that did not use generics, please see the doc for [old generated code](/docs/languages/go/generated-code-old). | ||
|
||
This page describes the code generated with the [grpc plugin](https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc), `protoc-gen-go-grpc`, |
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.
Let's reverse the order of these two paragraphs, combine them into one, and make them flow better.
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.
Done
@@ -3,6 +3,7 @@ title: Generated-code reference | |||
linkTitle: Generated code | |||
weight: 95 | |||
--- | |||
The following documents the latest generated code guide which uses generics. For documentation about the older version of generated code that did not use generics, please see the doc for [old generated code](/docs/languages/go/generated-code-old). |
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.
Call out that this version of the generated code is the default, and that the old documentation is available for people who are looking at old generated code, and that it's temporarily possible to get this behavior by setting the flag.
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.
Done.
@@ -0,0 +1,189 @@ | |||
--- |
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.
Is this just a copy/paste of the old markdown file? If so I won't review the language at all.
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.
Yes, it is an exact copy of the old markdown file.
@@ -0,0 +1,189 @@ | |||
--- | |||
title: Generated-code(non-generic) reference |
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.
Please put spaces before opening parentheses in text.
Maybe call this "legacy" instead?
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.
Done
|
||
This page describes the code generated with the [grpc plugin](https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc), `protoc-gen-go-grpc`, | ||
when compiling `.proto` files with `protoc`. | ||
This page describes the code generated when compiling `.proto` files with `protoc`, using the `protoc-gen-go-grpc` [grpc plugin](https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc). This latest version of generated code uses generics by default. If you're working with older generated code that doesn't use generics, you can find the relevant documentation [here](/docs/languages/go/generated-code-old). While we encourage using this latest version with generics, you can temporarily revert to the old behavior by setting the `useGenericStreams` flag to `false`. |
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.
Please update the issue about removing this flag to include some notes that we need to update this documentation when the flag is removed.
|
||
You can find out how to define a gRPC service in a `.proto` file in [Service definition](/docs/what-is-grpc/core-concepts/#service-definition). | ||
|
||
To implement streaming in gRPC, refer directly to the [ServerStream](https://pkg.go.dev/google.golang.org/grpc#ServerStream) and [ClientStream](https://pkg.go.dev/google.golang.org/grpc#ClientStream) documentation, which provides comprehensive instructions for effectively using these types in your service methods. |
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 links to the types that are embedded in the generic streaming types. Better documentation for using the generic streaming types is present on the generic streaming types themselves.
This doc should call out the documentation on those generic streaming types. The doc strings for the generic streaming types already reference ClientStream and ServerStream.
Fixes task 3 : grpc-go issue #1894 - update the grpc.io docs to use generics instead of generated interfaces.