-
Notifications
You must be signed in to change notification settings - Fork 147
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
fix: stop using GRPC channels after RST_STREAM #1373
Conversation
629507b
to
ad06daf
Compare
Codecov Report
@@ Coverage Diff @@
## master #1373 +/- ##
==========================================
- Coverage 98.51% 98.51% -0.01%
==========================================
Files 32 32
Lines 19440 19464 +24
Branches 1371 1375 +4
==========================================
+ Hits 19152 19174 +22
- Misses 284 286 +2
Partials 4 4
Continue to review full report at Codecov.
|
dev/src/pool.ts
Outdated
@@ -140,10 +149,19 @@ export class ClientPool<T> { | |||
* @private | |||
*/ | |||
private shouldGarbageCollectClient(client: T): boolean { | |||
// Don't garbagae collect clients that have active requests. |
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.
nit: typo
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.
Fixed
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.
LGTM with a minor nit
This PR is based on a theory (based on some anecdotal evidence) that once a GRPC Channel receives the first RST_STREAM error it will only every throw RST_STREAM errors. This PR changes our client pool to mark any GAX client that throws RST_STREAM as failed and ensures that the SDK spins up a new client for the next request.
Addresses #1023