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

cleanup: replace dial with newclient #7970

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

janardhanvissa
Copy link
Contributor

Partially address: #7049

RELEASE NOTES: None

Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.15%. Comparing base (724f450) to head (0068eaa).

Files with missing lines Patch % Lines
profiling/cmd/remote.go 0.00% 2 Missing ⚠️
balancer/grpclb/grpclb_remote_balancer.go 66.66% 1 Missing ⚠️
internal/stubserver/stubserver.go 66.66% 1 Missing ⚠️
xds/internal/test/e2e/e2e.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7970      +/-   ##
==========================================
+ Coverage   82.05%   82.15%   +0.10%     
==========================================
  Files         381      381              
  Lines       38539    38542       +3     
==========================================
+ Hits        31622    31665      +43     
+ Misses       5602     5567      -35     
+ Partials     1315     1310       -5     
Files with missing lines Coverage Δ
admin/test/utils.go 61.53% <100.00%> (ø)
balancer/rls/control_channel.go 93.18% <100.00%> (+0.05%) ⬆️
...ntials/alts/internal/handshaker/service/service.go 69.23% <100.00%> (ø)
resolver/manual/manual.go 100.00% <100.00%> (ø)
balancer/grpclb/grpclb_remote_balancer.go 84.66% <66.66%> (+0.97%) ⬆️
internal/stubserver/stubserver.go 81.06% <66.66%> (+0.14%) ⬆️
xds/internal/test/e2e/e2e.go 0.00% <0.00%> (ø)
profiling/cmd/remote.go 0.00% <0.00%> (ø)

... and 22 files with indirect coverage changes

if err != nil {
logger.Fatalf("gRPC Client: failed to dial the server at %v: %v", *serverAddr, err)
logger.Fatalf("gRPC Client: NewClient() failed %v: %v", *serverAddr, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Fatalf("gRPC Client: NewClient() failed %v: %v", *serverAddr, err)
logger.Fatalf("gRPC Client: failed to create a client for server at %q: %v", *serverAddr, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
logger.Fatalf("Fail to dial: %v", err)
logger.Fatalf("NewClient() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Fatalf("NewClient() failed: %v", err)
logger.Fatalf("Failed to create a client: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
errorLog.Fatalf("Fail to dial: %v", err)
errorLog.Fatalf("NewClient() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errorLog.Fatalf("NewClient() failed: %v", err)
errorLog.Fatalf("Failed to create a client: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
logger.Fatalf("Fail to dial: %v", err)
logger.Fatalf("NewClient() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Fatalf("NewClient() failed: %v", err)
logger.Fatalf("Failed to create a client: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
log.Fatalf("Fail to dial: %v", err)
log.Fatalf("NewClient() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Fatalf("NewClient() failed: %v", err)
log.Fatalf("Failed to create a client: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
logger.Fatalf("Fail to dial: %v", err)
logger.Fatalf("NewClient() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Fatalf("NewClient() failed: %v", err)
logger.Fatalf("Failed to create a client: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
logger.Fatalf("Fail to dial %v: %v", uris[i], err)
logger.Fatalf("NewClient() failed %v: %v", uris[i], err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Fatalf("NewClient() failed %v: %v", uris[i], err)
logger.Fatalf("Failed to create a client for server %q: %v", uris[i], err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
logger.Errorf("cannot dial %s: %v", *flagAddress, err)
logger.Errorf("NewClient() failed %s: %v", *flagAddress, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Errorf("NewClient() failed %s: %v", *flagAddress, err)
logger.Fatalf("Failed to create a client for server %q: %v", *flagAddress, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -110,7 +110,7 @@ func (r *Resolver) UpdateState(s resolver.State) {
defer r.mu.Unlock()
var err error
if r.CC == nil {
panic("cannot update state as grpc.Dial with resolver has not been called")
panic("cannot update state as grpc.NewClient with resolver has not been called")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot update state as channel has not exited IDLE state

Update throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -260,10 +260,11 @@ func (lb *lbBalancer) newRemoteBalancerCCWrapper() error {
// The grpclb server addresses will set field ServerName, and creds will
// receive ServerName as authority.
target := lb.manualResolver.Scheme() + ":///grpclb.subClientConn"
cc, err := grpc.Dial(target, dopts...)
cc, err := grpc.NewClient(target, dopts...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfawley do you know if this non-test usage can be effected by #7556? If so, should we hold-off on making this change?

Similar question for the following files:

  • balancer/rls/control_channel.go
  • credentials/alts/internal/handshaker/service/service.go

@arjan-bal arjan-bal added this to the 1.70 Release milestone Dec 26, 2024
@arjan-bal arjan-bal added the Type: Internal Cleanup Refactors, etc label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants