-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 #7068
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7068 +/- ##
==========================================
+ Coverage 81.24% 82.36% +1.12%
==========================================
Files 345 305 -40
Lines 33941 31389 -2552
==========================================
- Hits 27574 25853 -1721
+ Misses 5202 4464 -738
+ Partials 1165 1072 -93 |
Nice work on the PR. Thanks for taking a look at this. However while grep-ing, I still see more usages of grep output
|
@arvindbr8 a polite request for clarity on |
@silaselisha -- Thanks for bringing that issue to our notice. Seems like there is a bug in NewClient that is causing a regression. Investigating it here |
But that shouldn't necessarily block your PR |
@arvindbr8 I'll look into issue your raised, to learn the dynamics of the |
I turned up the logs with grpc-go/examples/features/orca/client/main.go Lines 103 to 106 in 4ec8307
In the case of the Github VM (and in my local machine) the dns resolver returned both the ipv6 and ipv4 address. It's unclear why we need that since the example should work even if there are more addresses. I would just remove that conditional in |
@arvindbr8 if both ipv4 and ipv6 are returned, is it possible to just change the condition to |
Its not deterministic because it depends on the machine you run the test in and the ENI(s) configured. I would just get rid of the conditional there. |
@arvindbr8 since |
Sure. I'm also okay even if we decide to do this in increments. |
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
@arvindbr8 a polite request you review this issue here. |
@silaselisha -- Seems like maybe you didnt push your branch to remote? I'm not seeing a new commit after the last review. I'm particular about this comment which would help satisfy Testing (extras) /orca example |
I closed #7075 because the error was with the way the test was setup. We shouldnt ideally be expecting the resolver to only return 1 address. That's why I mentioned in the comment that the fix for the test can be along with this PR. Ideally @dfawley mentioned how he would like to split the update in #7049. Your PR seems like it does tackle most of (1) and (2). This is better for tracking. If you don't prefer to do that, you can revert your changes to |
I don't mind including that piece in this PR, I am working on the missing piece, and I'll push the changes asp. |
@arvindbr8 I made the necessary changes but I now the PR it has more unnecessary commits |
Something is wrong with your fork's git repo, apparently. If you can't figure it out, worst case: you can make a new directory and (assuming |
@silaselisha -- I've addressed my comments and pushed a new commit to your branch. I think it looks good now. Per policy we would want one more review on the PR from our maintainers. cc: @zasweq, could you please take a pass at the documentation update and the other diffs |
Giving to Abhishek since he's on call. |
LGTM! |
Thanks for the PR @silaselisha. |
grpc.Dial
togrpc.NewClient
while some tests were left to use the deprecatedgrpc.Dial
RELEASE NOTES: none