cleanup: replace dial with newclient - #8196
Conversation
|
Is this PR ready for review? |
yes, please |
6458128 to
d672f29
Compare
This reverts commit 79f6f35.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
There are a lot of unnecessary log message changes added in this PR. This makes it really hard to spot and review actual code changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8196 +/- ##
==========================================
+ Coverage 82.06% 82.08% +0.02%
==========================================
Files 410 419 +9
Lines 40233 41988 +1755
==========================================
+ Hits 33018 34467 +1449
- Misses 5854 6046 +192
- Partials 1361 1475 +114
🚀 New features to boost your workflow:
|
|
I've addressed all the comments by reverting the changes related to log message updates and fixing the race condition by calling |
|
@arjan-bal I think Janardhan has reverted the unnecessary changes in the PR. It should be ready for review now. |
| // | ||
| // Note that using this credentials with per-RPC credentials which require | ||
| // transport security is incompatible and will cause grpc.Dial() to fail. | ||
| // transport security is incompatible and will cause grpc.NewClient() to fail. |
There was a problem hiding this comment.
So this line needs to be updated to say that RPCs will fail and not grpc.NewClient().
| // Note that using this DialOption with per-RPC credentials (through | ||
| // WithCredentialsBundle or WithPerRPCCredentials) which require transport | ||
| // security is incompatible and will cause grpc.Dial() to fail. | ||
| // security is incompatible and will cause grpc.NewClient() to fail. |
There was a problem hiding this comment.
So this line needs to be updated to say that RPCs will fail and not grpc.NewClient().
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
We should not create a context in this method, instead pass a context created in the original test function. This ensures the entire test has 10 secs to run. Presently if a test created channels in a for loop, the overall test timeout will get multiplied.
| te.t.Fatalf("Dial(%q) = %v", scheme+te.srvAddr, err) | ||
| te.t.Fatalf("grpc.NewClient() failed(%q) = %v", scheme+te.srvAddr, err) | ||
| } | ||
| te.cc.Connect() |
There was a problem hiding this comment.
Not actionable: Normally I would suggest moving the Connect() call to the calling functions because most of the callers wouldn't need it as they're making RPCs. In this case, there are 116 references, so we can keep the existing behaviour.
|
@arjan-bal The changes are done, it's ready for review |
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM with one minor comment.
| te.cc, err = grpc.NewClient(te.srvAddr, opts...) | ||
| if err != nil { | ||
| te.t.Fatalf("Dial(%q) = %v", te.srvAddr, err) | ||
| te.t.Fatalf("grpc.NewClient() failed(%q) = %v", te.srvAddr, err) |
There was a problem hiding this comment.
(%q) should be logged just after the function name as it's an argument.
| te.cc, err = grpc.NewClient(scheme+te.srvAddr, opts...) | ||
| if err != nil { | ||
| te.t.Fatalf("Dial(%q) = %v", scheme+te.srvAddr, err) | ||
| te.t.Fatalf("grpc.NewClient() failed(%q) = %v", scheme+te.srvAddr, err) |
There was a problem hiding this comment.
| te.t.Fatalf("grpc.NewClient() failed(%q) = %v", scheme+te.srvAddr, err) | |
| te.t.Fatalf("grpc.NewClient(%q) failed: %v", scheme+te.srvAddr, err) |
5 participants
addresses: #7049
RELEASE NOTES: None