-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
xdsclient: invoke connectivity failure callback only after all listed servers have failed #8075
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8075 +/- ##
==========================================
+ Coverage 82.10% 82.27% +0.16%
==========================================
Files 387 387
Lines 39067 39051 -16
==========================================
+ Hits 32076 32128 +52
+ Misses 5662 5605 -57
+ Partials 1329 1318 -11
|
xds/internal/xdsclient/authority.go
Outdated
for _, rType := range a.resources { | ||
for _, state := range rType { | ||
for watcher := range state.watchers { | ||
watcher := watcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is not needed post-Go 1.22?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. Thanks.
} | ||
return | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why false here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't to notify watchers in this case, because we have a higher priority server failing (during a retry after backoff), while a lower priority server is either successfully connected or in the process of connecting.
t.Fatalf("Failed to create gRPC client: %v", err) | ||
} | ||
defer cc.Close() | ||
cc.Connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we skip this? Otherwise it's possible the client fully connects before we even dispatch the RPC to the channel. OK, that's very unlikely, but there should be no need for this call, anyway AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Done. Thanks.
0417b07
to
b763853
Compare
… servers have failed (grpc#8075)
Implements the update to gRFC A71 made in grpc/proposal#473.
RELEASE NOTES: