Add documentation on some anti-patterns

[ulascansenturk]

Created error handling documentation,

To rely on RPC errors instead of

grpc.FailOnNonTempDialError

grpc.WithBlock

grpc.WithReturnConnectionError

#6028

RELEASE NOTES: N/A

[easwars]

@ulascansenturk : Could you please wrap the lines at 80-cols. Thank you.

[dfawley]

I would like to begin a document that explains why these options are not recommended by explaining that grpc.Dial is not actually a "dialing" function at all. It creates a ClientConn, which is a persistent, managed pool of connections that automatically reconnects as needed. The reason these options are not useful is that errors encountered during the first Dial are no different from those that occur 1 second after the initial connection succeeds, and your application needs to handle both of those cases. It's for that reason that we don't recommend specially handling the initial connection error vs. any other one (vs. "WithBlock...may end up waiting indefinitely" and "miss opportunities to recover").

[dfawley]

Please also wrap all lines at 80 columns when you can, which will make it easier for us to read & review.

[ulascansenturk]

Please also wrap all lines at 80 columns when you can, which will make it easier for us to read & review.

Done

[dfawley]

Sorry again for the late reply. This looks really good, just a few more comments.

[ulascansenturk]

added to both the first references at the beginning and the references at the very end.
@dfawley

[dfawley]

Could you please add the references in the other direction? It would be nice if the godoc for those functions pointed to this document.

[ulascansenturk]

For better understanding, do you want me to add godoc references to all other docstrings?

[dfawley]

I would like a link to this document from

grpc-go/dialoptions.go

Line 297 in 2997e84

// connecting the server happens in background.

(etc). I recognize this document has no current link, so it will look broken in the PR, but it will work when it's merged. (https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md)

[ulascansenturk]

Got it, i'll prepare new PR for that, or should I do it in this pull request?

[dfawley]

You can do it in this one, thanks.

[dfawley]

One other last minor thing. Otherwise LGTM!

[dfawley]

It seems some of the wrapping got messed up here. Please take a final pass and make sure it's all lined up correctly, thanks.

[dfawley]

How about:

//
// Use of this feature is not recommended.  For more information, please see: <link>

?

[dfawley]

Please move this above the // # Experimental line and use the same text as above. (and similar request below)

[dfawley]

@easwars can you review the changes & re-approve?

[oguzyildizz]

The reason these options are not useful is that errors encountered during the first Dial are no different from those that occur 1 second after the initial connection succeeds

@dfawley what if we want to catch configuration mistakes and don't care about the transient ones? Then WithBlock becomes useful before marking the container ready, no? What would you suggest instead?

[dfawley]

@dfawley what if we want to catch configuration mistakes and don't care about the transient ones? Then WithBlock becomes useful before marking the container ready, no?

So in this case, WithBlock may actually cause you to fail in the event of a transient problem as well. If the dependent service happens to be down or unreachable at startup due to a transient problem with that service or your network, you will timeout and fail despite there being no configuration error.

For configuration errors, there are many other kinds of configuration problems and other errors that you couldn't catch this way anyway. E.g. you may connect to the wrong service, so the connection would succeed, but all RPCs would fail with NOT_IMPLEMENTED.

What would you suggest instead?

Yeah, as I said in #6034 (comment): "Rather than fixate on "don't do these things", we should ideally be explaining "here's how to do the things you need to do"", so I fully expected these sorts of questions. 😬

One way to detect and mitigate configuration errors is to use a phased rollout/canary process, and instrument monitoring in your applications. You can always just perform your RPCs using the channel, and the RPCs will fail with UNAVAILABLE when there are connection problems, and the error message should indicate the reason for the connection failure. Any dispatched RPCs will block until there is a READY connection or until the initial connection attempt has errored. (You should also typically not use the confusingly-named WaitForReady call option, as this will make the RPC block even after connection errors occur, until the RPC's deadline is reached.)

[dfawley]

cc @temawi