examples/features/retry: Improve docstring #7331
Conversation
| @@ -47,7 +47,9 @@ var ( | |||
| }]}` | |||
| ) | |||
|
|
|||
| // use grpc.WithDefaultServiceConfig() to set service config | |||
| // use grpc.WithDefaultServiceConfig() to set service config. Ideal | |||
There was a problem hiding this comment.
slight improvement: "However, the recommended approach
is to fetch the retry configuration from the name resolver rather than defining it on the client side."
There was a problem hiding this comment.
I like purnesh' exact wording a bit more than what you ended up with.
Also, if this is a documentation string it goes above the function definition but it should start with retryDial as outlined in https://tip.golang.org/doc/comment, otherwise you shoud probably move it one line below, above grpc.Dial. While you're here, can you also replace retryDial with newClientWithRetries or something like that, since it doesn't use dial anymore?
There was a problem hiding this comment.
thAnks @atollena, @purnesh42H for the detailed review, i have updated all the suggestion.
| // service config, and creates the channel. However, the recommended | ||
| // approach is to fetch the retry configuration from the name resolver | ||
| // rather than defining it on the client side. | ||
| func newClientWithRetries() (*grpc.ClientConn, error) { |
There was a problem hiding this comment.
I don't understand the need for this function newClientWithRetries. I would recommend getting rid of it, and directly calling grpc.NewClient from main.
| // use grpc.WithDefaultServiceConfig() to set service config | ||
| func retryDial() (*grpc.ClientConn, error) { | ||
| // newClientWithRetries uses grpc.WithDefaultServiceConfig() to set | ||
| // service config, and creates the channel. However, the recommended |
There was a problem hiding this comment.
A minor improvement:
However, the recommended approach is to fetch the retry configuration (which is part of the service config) from the name resolver rather than defining it on the client side.
This issue #7275 talks about the example where the service config is being passed to the NewClient, but ideally the retry config should be accessed via the name resolver instead of defining it from client side. This PR adds the recommended approach.
RELEASE NOTES: n/a