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

xds/ringhash: update connectivity state aggregation, and make sure at least one SubConn is connecting in TF #5338

Merged
merged 3 commits into from
May 18, 2022

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented May 6, 2022

grpc/proposal#296

RELEASE NOTES: N/A

…ggregation, and make sure on SubConn is connecting in TF
@menghanl menghanl requested a review from dfawley May 6, 2022 21:13
@menghanl menghanl added the Type: Behavior Change Behavior changes not categorized as bugs label May 6, 2022
@menghanl menghanl added this to the 1.47 Release milestone May 6, 2022
@menghanl menghanl changed the title xds/ringhash: update connectivity state aggregation, and make sure on SubConn is connecting in TF xds/ringhash: update connectivity state aggregation, and make sure at least one SubConn is connecting in TF May 10, 2022
@@ -147,8 +156,10 @@ func (sc *subConn) effectiveState() connectivity.State {
// it's Connect() will be triggered. If the SubConn state is already Idle, it
// will just call Connect().
func (sc *subConn) queueConnect() {
fmt.Printf(" ===== queue connect for %v\n", sc)
Copy link
Member

Choose a reason for hiding this comment

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

Delete (there are at least a few of these in here)

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

@@ -417,6 +463,12 @@ func (cse *connectivityStateEvaluator) recordTransition(oldState, newState conne
updateVal := 2*uint64(idx) - 1 // -1 for oldState and +1 for new.
cse.nums[state] += updateVal
}
if oldState == connectivity.Shutdown {
Copy link
Member

Choose a reason for hiding this comment

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

Can you ever transition from shutdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a new SubConn is created, we record a shutdown->idle transition.
(And I believe we do the same in roundrobin)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that makes sense thanks.

@dfawley dfawley assigned menghanl and unassigned dfawley May 17, 2022
@menghanl menghanl assigned dfawley and unassigned menghanl May 17, 2022
@@ -417,6 +463,12 @@ func (cse *connectivityStateEvaluator) recordTransition(oldState, newState conne
updateVal := 2*uint64(idx) - 1 // -1 for oldState and +1 for new.
cse.nums[state] += updateVal
}
if oldState == connectivity.Shutdown {
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that makes sense thanks.

@dfawley dfawley assigned menghanl and unassigned dfawley May 17, 2022
@menghanl menghanl merged commit 333a441 into grpc:master May 18, 2022
@menghanl menghanl deleted the ringhash_connectivity_fix branch May 18, 2022 17:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants