client: Change connectivity state to CONNECTING when creating the name resolver #8710
Conversation
easwars
added
Type: Bug
Area: Client
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8710 +/- ##
==========================================
- Coverage 83.28% 83.20% -0.09%
==========================================
Files 416 419 +3
Lines 32267 32431 +164
==========================================
+ Hits 26874 26983 +109
- Misses 4019 4061 +42
- Partials 1374 1387 +13
🚀 New features to boost your workflow:
|
dial_test.go
Outdated
|
|
||
| func (s stringerVal) String() string { return s.s } | ||
|
|
||
| const errResolverBuildercheme = "test-resolver-build-failure" |
resolver_wrapper.go
Outdated
| // https://github.com/grpc/grpc/blob/master/doc/connectivity-semantics-and-api.md | ||
| // defines CONNECTING as follows: | ||
| // - The channel is trying to establish a connection and is waiting to | ||
| // make progress on one of the steps involved in name resolution, TCP | ||
| // connection establishment or TLS handshake. This may be used as the | ||
| // initial state for channels upon creation. | ||
| // | ||
| // We are starting the name resolver here as part of exiting IDLE, so | ||
| // transitioning to CONNECTING is the right thing to do. |
There was a problem hiding this comment.
IMO comments should be short and to the point.
Short comments make the code take up less space, which makes it easier to read and understand. Long comments make long functions extremely long and not fit on the page.
Honestly, I think a comment for this action isn't even necessary. But if you think we need one, this could be:
// Set state to CONNECTING before building the name resolver
// so the channel does not remain in IDLE.| if state := cc.GetState(); state != connectivity.Idle { | ||
| t.Fatalf("Expected initial state to be IDLE, got %v", state) | ||
| } |
There was a problem hiding this comment.
The AwaitState above already tested this IIUC
| // Ensure that the client is in IDLE before connecting. | ||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
| testutils.AwaitState(ctx, t, cc, connectivity.Idle) |
There was a problem hiding this comment.
This doesn't need an Await right? It should just check the current state, and never wait for changes, as we know it starts idle.
There was a problem hiding this comment.
That's true. Moved the check for the current state to here, and got rid of the Await.
| for _, wantState := range wantStates { | ||
| waitForState(ctx, t, stateCh, wantState) | ||
| if wantState == connectivity.Idle { | ||
| tt.exitIdleFunc(ctx, cc) |
There was a problem hiding this comment.
Can we make this test the actual RPC error when we use an RPC to exit idle?
There was a problem hiding this comment.
Done. I changed the test a little to make that happen.
But as part of doing that, I realized a thing or two:
- The RPC is not failing with a status error, because the
idle.Manager.ExitIdleModewhich is called when an RPC has to kick the channel out ofIDLE, does not embed the error returned fromClientConn.ExitIdleMode. But if we make it embed the error, we will have to return a status error from the latter, which is doable. But that brings the following questions:- What code do we return? I'm torn between
UnavailableandInternal, and leaning towards the latter - This would also make
Dialfail with a status error which I find a little odd.
- What code do we return? I'm torn between
Thoughts?
resolver_wrapper.go
Outdated
| // | ||
| // We are starting the name resolver here as part of exiting IDLE, so | ||
| // transitioning to CONNECTING is the right thing to do. | ||
| ccr.cc.csMgr.updateState(connectivity.Connecting) |
There was a problem hiding this comment.
Could this (and the error handling below) go into exitIdleMode instead? It might make more sense to be in the channel directly, rather than in this sub-component.
3 participants
Fixes #7686
Current Behavior
Connectbeing called or because of an RPC), if name resolver creation fails, we stay in IDLE.New Behavior
Connectbeing called or because of an RPC), we have already moved to CONNECTING (because of the previous bullet point). If name resolver creation fails, we will move to TRANSIENT_FAILURE and start the idle timer and move back to IDLE when the timer firesRELEASE NOTES: