Skip to content

Conversation

MakMukhi
Copy link
Contributor

fixes #1208
#1359

@MakMukhi MakMukhi requested review from dfawley and menghanl July 21, 2017 23:09
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks great, mostly just nits.

clientconn.go Outdated
}

// connectivityStateEvaluator gets updated by addrConns when their
// stats transition, based on which it evaluates the state of
Copy link
Member

Choose a reason for hiding this comment

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

*states

clientconn.go Outdated
// connectivityStateEvaluator gets updated by addrConns when their
// stats transition, based on which it evaluates the state of
// ClientConn.
// Note: This code will eventully sit in the blanacer in the new design.
Copy link
Member

Choose a reason for hiding this comment

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

*eventually

clientconn.go Outdated
// before any addrConn is created ClientConn is in idle state. In the end when ClientConn
// closes it is in Shutdown state.
// TODO Note that in later releases, a ClientConn with no activity will be put into an Idle state.
func (csEvltr *connectivityStateEvaluator) recordTransition(oldState, newState ConnectivityState) {
Copy link
Member

Choose a reason for hiding this comment

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

csEvltr -> c? cse?

clientconn.go Outdated
ac.down(downErrorf(false, true, "%v", errNetworkIO))
ac.down = nil
}
oldState := ac.state
Copy link
Member

Choose a reason for hiding this comment

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

How about this:

updateState := func(new ConnectivityState) {
  ac.csEvltr.recordTransition(ac.state, new)
  ac.state = new
}
....
updateState(Connecting)
...
updateState(TransientFailure)
...

defer cc.Close()
wantState := Ready
if state, ok := assertState(wantState, cc); !ok {
t.Fatalf("asserState(%s) = %s, false, want %s, true", wantState, state, wantState)
Copy link
Member

Choose a reason for hiding this comment

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

*assertState

clientconn.go Outdated
// connectivityStateEvaluator gets updated by addrConns when their
// states transition, based on which it evaluates the state of
// ClientConn.
// Note: This code will eventually sit in the blanacer in the new design.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: balancer is typo'ed

clientconn.go Outdated
// recordTransition records state change happening in every addrConn and based on
// that it evaluates what state the ClientConn is in.
// It can only transition between Ready, Connecting and TransientFailure. Other states,
// Idle and Shutdown are transitioned into by ClientConn; in the begining of the conneciton
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: begining, conneciton

}

// updateState updates the ConnectivityState of ClientConn.
// If there's a change it notifies goroutines waiting on state change to
Copy link
Contributor

Choose a reason for hiding this comment

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

it can only notify 1 goroutine, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although rare, but it's possible multiple goroutines can make a call to WaitForStateChange

clientconn.go Outdated
err = cc.resetAddrConn(a, false, nil)
}
if err != nil {
grpclog.Warningf("Error creating conneciton to %v. Err: %v", a, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: connection

@MakMukhi MakMukhi merged commit a5d184a into grpc:master Jul 24, 2017
@menghanl menghanl added 1.6 Type: Feature New features or improvements in behavior labels Aug 24, 2017
@dfawley dfawley modified the milestone: 1.6 Release Aug 28, 2017
@MakMukhi MakMukhi deleted the conn_state branch May 4, 2018 02:01
@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

channel state changes

4 participants