cleanup: replace dial with newclient #7967

janardhanvissa · 2024-12-25T11:52:59Z

Partially address: #7049

RELEASE NOTES: None

codecov · 2024-12-25T11:57:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.26%. Comparing base (f35fb34) to head (e253082).
Report is 17 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7967      +/-   ##
==========================================
+ Coverage   82.17%   82.26%   +0.09%     
==========================================
  Files         381      383       +2     
  Lines       38546    38776     +230     
==========================================
+ Hits        31676    31900     +224     
+ Misses       5560     5555       -5     
- Partials     1310     1321      +11

see 51 files with indirect coverage changes

arjan-bal · 2025-01-09T10:14:56Z

test/clientconn_state_transition_test.go

 		grpc.WithTransportCredentials(insecure.NewCredentials()),
 		grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, stateRecordingBalancerName)))
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer client.Close()
-
+	client.Connect()


There is a call to testutils.StayConnected below, do we need to call connect here? Please consider this every time you replace Dial with NewClient across all PRs.

getting the below error without connect() call

timed out waiting for state 0 (CONNECTING) in flow [CONNECTING READY IDLE CONNECTING]

arjan-bal · 2025-01-09T10:16:18Z

test/gracefulstop_test.go

-	ctx, dialCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
-	defer dialCancel()
-	cc, err := grpc.DialContext(ctx, "", grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(d))
+	cc, err := grpc.NewClient("localhost:0", grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(d))


Use passthrough instead of changing the hostname. I've mentioned the usage of custom dialers before.

arjan-bal · 2025-01-09T10:18:35Z

xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go

 	}
+	cc.Connect()


Instead of calling cc.Connect here make the callers of setupForSecurityTests call it only if needed.

arjan-bal · 2025-01-09T10:19:00Z

xds/internal/balancer/cdsbalancer/cdsbalancer_test.go

 	}
+	cc.Connect()


Instead of calling cc.Connect here make the callers of setupWithManagementServerAndListener call it only if needed.

Similar comment for other setup functions.

arjan-bal · 2025-01-09T10:22:41Z

test/xds/xds_server_serving_mode_test.go

+	if cc1, err = grpc.NewClient(lis1.Addr().String(), grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())); err != nil {
+		t.Fatal("Failed to create clientConn to a server in \"not-serving\" state")


This is not the same assertion as before. You need to make an RPC and check that it fails.

This reverts commit 506fd98, reversing changes made to d520f73.

…pc-go" This reverts commit 0486355, reversing changes made to 7ddbe46.

…rpc-go" This reverts commit fd2860b.

This reverts commit f1ec518.

This reverts commit 506fd98, reversing changes made to d520f73.

…rpc-go" This reverts commit fd2860b.

This reverts commit bf780fe.

…to replace-dial-with-newclient

This reverts commit 658b745.

arjan-bal · 2025-01-23T10:14:49Z

Please ensure tests pass before raising the PR.

janardhanvissa · 2025-01-23T10:26:40Z

Please ensure tests pass before raising the PR.

Yeah sure, but before pushing the changes verified locally, there were no issues.

arjan-bal · 2025-01-23T10:59:29Z

clientconn_test.go

@@ -313,9 +315,9 @@ func (s) TestCloseConnectionWhenServerPrefaceNotReceived(t *testing.T) {
 			break
 		}
 	}()
-	client, err := Dial(lis.Addr().String(), WithTransportCredentials(insecure.NewCredentials()), withMinConnectDeadline(func() time.Duration { return time.Millisecond * 500 }))


The test should be renamed now, replacing Dial with NewClient. Same change throughout.

Yeah, I'm about to ask you that question, Thanks for the clarification.

arjan-bal · 2025-01-23T11:13:03Z

clientconn_test.go

+	// SwitchBalancer before NewAddress. There was no balancer created, this
+	// makes sure we don't call close on nil balancerWrapper.
+	sc := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)
+	r.InitialState(resolver.State{ServiceConfig: sc(`{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic.
+


This is changing the test behaviour. NewClient will not create a balancer unless RPC is made or connect is called. So "switching" balancer is essentially a no-op. Instead we should do the following:

Create NewClient

Call cc.Connect()

Call r.UpdateState() with the new LB config.

arjan-bal · 2025-01-23T11:14:34Z

clientconn_test.go

-
-		cc, err := Dial(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r))
+		sc := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)
+		go r.InitialState(resolver.State{ServiceConfig: sc(`{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic.
+		cc, err := NewClient(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r))
 		if err != nil {
-			t.Fatalf("failed to dial: %v", err)
+			t.Fatalf("Failed to create a client for server: %v", err)
 		}
 		// Send a new service config while closing the ClientConn.
 		go cc.Close()
-		go r.UpdateState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic.


This changes the scenario that was being tested. Instead of using r.InitialState keep the UpdateState calls. Just call cc.Connect() after creating the new client/

arjan-bal · 2025-01-23T11:16:46Z

clientconn_test.go

-	cc, err := Dial(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r))
+	// This make sure we don't create addrConn with empty address list.
+	r.InitialState(resolver.State{}) // This should not panic.
+
+	cc, err := NewClient(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r))
 	if err != nil {
-		t.Fatalf("failed to dial: %v", err)
+		t.Fatalf("Failed to create a client for server: %v", err)
 	}


The resolver is not created because NewClient is lazy. So the panic can't happen. Please consider what the test is trying to do and ensure the behviour is the same while making changes.

janardhanvissa · 2025-01-23T18:04:26Z

@arjan-bal Kindly please suggest the below queries:

Seems like there is some issue in TestClientUpdatesParamsAfterGoAway how to proceed on this. Please suggest this.
Do we need to replace Dial/DialContext with NewClient for the below tests also?
TestDialContextCancel
TestDialContextFailFast
TestDialWaitsForServerSettingsAndFails
TestWithTimeout
TestWithTransportCredentialsTLS

…rdhanvissa/grpc-go into replace-dial-with-newclient

janardhankrishna-sai added 2 commits December 23, 2024 07:09

replace dial with newclient

7ddbe46

replace dial with newclient

71dacb1

janardhankrishna-sai and others added 18 commits December 26, 2024 11:58

updating error msg and replacing dial with newclient

88c3f0f

updating error mdg

ceb1204

revert TestFederation_UnknownAuthorityInDialTarget

6be3cf0

updating fatal msg

f6cb17c

replace dial with newclient

9f02ddd

updating error msg

7fa6e5e

Merge branch 'master' of https://github.com/janardhanvissa/grpc-go

0486355

updating error msg

aec4527

updating test

8ea1edd

updating fatal msg

c7e8c3e

updating newclient target

baec474

updating newclient target

36deff4

updating fatal msg

c58db07

updating fatal msg

973d28a

updating fatal msg

c07189f

error msg updating

1fc9c6b

updating fatal msg

d520f73

Merge branch 'grpc:master' into master

53fa9cd

arjan-bal reviewed Jan 9, 2025

View reviewed changes

arjan-bal assigned janardhanvissa Jan 9, 2025

arjan-bal reviewed Jan 9, 2025

View reviewed changes

janardhankrishna-sai added 6 commits January 9, 2025 10:40

updating newclient err msg

506fd98

Revert "updating newclient err msg"

f1ec518

This reverts commit 506fd98, reversing changes made to d520f73.

Revert "Merge branch 'master' of https://github.com/janardhanvissa/gr…

fd2860b

…pc-go" This reverts commit 0486355, reversing changes made to 7ddbe46.

Reapply "Merge branch 'master' of https://github.com/janardhanvissa/g…

56c4730

…rpc-go" This reverts commit fd2860b.

Reapply "updating newclient err msg"

5c6eb74

This reverts commit f1ec518.

Revert "updating newclient err msg"

bf780fe

This reverts commit 506fd98, reversing changes made to d520f73.

janardhankrishna-sai and others added 16 commits January 9, 2025 11:55

Reapply "Merge branch 'master' of https://github.com/janardhanvissa/g…

74ea67a

…rpc-go" This reverts commit fd2860b.

Reapply "updating newclient err msg"

e7e95ce

This reverts commit bf780fe.

addressing review comments and reapply reverted changes

e24f0a5

addressing review comments and reapply reverted changes

aa049d9

Merge branch 'grpc:master' into master

4435b8a

Merge branch 'master' of https://github.com/janardhanvissa/grpc-go in…

6c69f73

…to replace-dial-with-newclient

adding comment

986e4e0

removing explicit client connection on setupAndDial

12936a7

updating explicit client connection

aa1434b

updating explicit client connection

0db5faa

updating explicit client connection

dad978a

replace dial with newclient

894eee7

updating fatal msg

8b8496e

reverting test update

b6f32f3

reverting test update

658b745

Revert "reverting test update"

f1102b3

This reverts commit 658b745.

replace dial with newclient

6a83587

arjan-bal reviewed Jan 23, 2025

View reviewed changes

renaming test from dial to newclient

81c4724

janardhankrishna-sai added 5 commits January 23, 2025 18:17

updating resolver state

a6630a7

replace dial with newclient

54fc7e8

Merge branch 'replace-dial-with-newclient' of https://github.com/jana…

ba3a12c

…rdhanvissa/grpc-go into replace-dial-with-newclient

removing empty line

0c6c9e2

updating newclient target

e253082

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: replace dial with newclient #7967

cleanup: replace dial with newclient #7967

janardhanvissa commented Dec 25, 2024

codecov bot commented Dec 25, 2024 •

edited

Loading

arjan-bal Jan 9, 2025

janardhanvissa Jan 9, 2025

arjan-bal Jan 9, 2025

janardhanvissa Jan 12, 2025

arjan-bal Jan 9, 2025

janardhanvissa Jan 12, 2025

arjan-bal Jan 9, 2025

arjan-bal Jan 9, 2025

janardhanvissa Jan 12, 2025

arjan-bal Jan 9, 2025

janardhanvissa Jan 12, 2025

arjan-bal commented Jan 23, 2025

janardhanvissa commented Jan 23, 2025

arjan-bal Jan 23, 2025

janardhanvissa Jan 23, 2025

janardhanvissa Jan 23, 2025

arjan-bal Jan 23, 2025

janardhanvissa Jan 23, 2025

arjan-bal Jan 23, 2025

janardhanvissa Jan 23, 2025

arjan-bal Jan 23, 2025

janardhanvissa Jan 23, 2025

janardhanvissa commented Jan 23, 2025

		if cc1, err = grpc.NewClient(lis1.Addr().String(), grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())); err != nil {
		t.Fatal("Failed to create clientConn to a server in \"not-serving\" state")

cleanup: replace dial with newclient #7967

Are you sure you want to change the base?

cleanup: replace dial with newclient #7967

Conversation

janardhanvissa commented Dec 25, 2024

codecov bot commented Dec 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal commented Jan 23, 2025

janardhanvissa commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janardhanvissa commented Jan 23, 2025

codecov bot commented Dec 25, 2024 •

edited

Loading