Skip to content
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

Update docs and examples and tests to use NewClient instead of Dial #7049

Open
3 tasks
dfawley opened this issue Mar 19, 2024 · 14 comments
Open
3 tasks

Update docs and examples and tests to use NewClient instead of Dial #7049

dfawley opened this issue Mar 19, 2024 · 14 comments

Comments

@dfawley
Copy link
Member

dfawley commented Mar 19, 2024

After #7010 we need to make sure all our documentation recommends NewClient. We should still retain some tests for Dial but the majority of them should probably be converted to use NewClient as well.

Tasks:

  • Tests cases that don't rely upon Dial (tests for WithTimeout/etc should keep using Dial).
  • In documentation, update from Dial to NewClient everywhere (note: this include http://github.com/grpc/grpc.io
  • Update anti-patterns doc to discuss the differences between Dial and NewClient and also mention WithTimeout specifically in the existing section about Dial.
@silaselisha
Copy link
Contributor

@dfawley I would like to work on this issue

@dfawley
Copy link
Member Author

dfawley commented Mar 26, 2024

Sounds good @silaselisha, that would be great -- we need to start only inside the repo for now, because we don't want to update any other documentation until our next release which includes the new function.

Note that we can't just do a trivial sed to replace grpc.Dial with grpc.NewClient (though maybe that's a decent way to start), because there are minor behavior differences that some of our tests probably (hopefully) will rely on.

@silaselisha
Copy link
Contributor

silaselisha commented Mar 26, 2024

@dfawley I'll start working on the example repo, and as mentioned I'll strictly traverse through the files just to manually replace grpc.Dial with grpc.NewClient.

@SamYuan1990
Copy link

SamYuan1990 commented Apr 4, 2024

To be specific, how to set timeout makes me confusing as this newclient function invoking WithTimeout in current test case which is marked as Deprecated... for details at #7093

@dfawley
Copy link
Member Author

dfawley commented Apr 4, 2024

Timeouts are not supported in NewClient. See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.

If you need timeouts, feel free to keep using Dial or DialContext; they are not going to be deleted in 1.x, they are just not a good idea to use per the information above.

@SamYuan1990
Copy link

Timeouts are not supported in NewClient. See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.

If you need timeouts, feel free to keep using Dial or DialContext; they are not going to be deleted in 1.x, they are just not a good idea to use per the information above.

can we remove Deprecated on Dial or DialContext to avoid lint error?

@SamYuan1990
Copy link

https://github.com/Hyperledger-TWGC/tape/actions/runs/8551020475/job/23429132147?pr=384#step:5:25

Error: SA1019: grpc.DialContext is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (staticcheck)

@dfawley
Copy link
Member Author

dfawley commented Apr 4, 2024

https://www.merriam-webster.com/dictionary/deprecate

to withdraw official support for or discourage the use of (something, such as a software product) in favor of a newer or better alternative

"Deprecated" seems appropriate here. You should ideally stop using it, or update your linter if that's not possible.

See also https://en.wikipedia.org/wiki/Deprecation:

deprecation is the discouragement of use of some terminology, feature, design, or practice, typically because it has been superseded or is no longer considered efficient or safe, without completely removing it or prohibiting its use. Typically, deprecated materials are not completely removed to ensure legacy compatibility or back up practice in case new methods are not functional in an odd scenario.

@SamYuan1990
Copy link

Timeouts are not supported in NewClient. See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.

If you need timeouts, feel free to keep using Dial or DialContext; they are not going to be deleted in 1.x, they are just not a good idea to use per the information above.

For people who want to use Timeout with currently Dial or DialContext been marked as Deprecated, the lint as a part of static scan will fail, breaks OpenSSF practices.

Hence, IMO, which means gprc is not going to support timeout feature any more?

@dfawley
Copy link
Member Author

dfawley commented Apr 4, 2024

breaks OpenSSF practices

What practices exactly are you referring to? Do you have documentation that you can refer to?

We will continue to test and support these functions. However, we don't recommend that you use them as they reinforce anti-patterns, hence they are marked as deprecated.

@SamYuan1990
Copy link

// The DialOptions returned by WithBlock, WithTimeout, and

.... ok I got the point. The current code comments guide me to I can use WithTimeout to return a DialOption.
but in fact I can't as the next line is

// WithReturnConnectionError are ignored by this function.

to reduce confusing, please improve the comments, and remove the test case https://github.com/grpc/grpc-go/blob/master/clientconn_test.go#L390 as we are not going to support timeout feature.

@dfawley
Copy link
Member Author

dfawley commented Apr 4, 2024

remove the test case https://github.com/grpc/grpc-go/blob/master/clientconn_test.go#L390 as we are not going to support timeout feature.

As I've said several times now, we are going to support the timeout feature. We simply recommend against its use (as we have for about as long as I've been the lead for gRPC-Go, 6+ years). We cannot delete tests for it, otherwise we'd risk breaking users when making changes.

@SamYuan1990
Copy link

remove the test case https://github.com/grpc/grpc-go/blob/master/clientconn_test.go#L390 as we are not going to support timeout feature.

As I've said several times now, we are going to support the timeout feature. We simply recommend against its use (as we have for about as long as I've been the lead for gRPC-Go, 6+ years). We cannot delete tests for it, otherwise we'd risk breaking users when making changes.

then, please, shall we make this test case without deprecated functions as Dial, WithTimeout, WithBlock?
For current, I don't want to change my lint setting hence I will switch to the NewClient without timeout, and go sleep as already over mid night local time.

@dfawley
Copy link
Member Author

dfawley commented Apr 4, 2024

then, please, shall we make this test case without deprecated functions as Dial, WithTimeout, WithBlock?

I don't know how to do what you're requesting. The only options are: 1. to remove "Deprecated" from the docstring of these functions, but we don't want to do that, as that's the way we notify users that they are not recommended; or 2. to not test these functions at all, but we can't do that, as we need tests to confirm they are working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants