refactor: drop dependency on @google-cloud/common-grpc#554
refactor: drop dependency on @google-cloud/common-grpc#554JustinBeckwith merged 3 commits intomasterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #554 +/- ##
==========================================
+ Coverage 97.79% 97.81% +0.02%
==========================================
Files 10 11 +1
Lines 1313 1329 +16
Branches 347 349 +2
==========================================
+ Hits 1284 1300 +16
Partials 29 29
Continue to review full report at Codecov.
|
src/index.ts
Outdated
| noResponseRetries: 0, | ||
| objectMode: true, | ||
| shouldRetryFn: (Service as any).shouldRetryRequest_, | ||
| shouldRetryFn: r => [429, 500, 502, 503].indexOf(r.code) > -1, |
There was a problem hiding this comment.
We should refactor this to use gax for handling retries, it's perfectly capable of doing it. (I didn't read this code so it might be harder than it sounds)
There was a problem hiding this comment.
Hrumph. The comments in the code point to here, which makes me think maybe we don't support retries natively here yet?
https://github.com/googleapis/gax-nodejs/blob/master/src/streamingCalls/streamDescriptor.ts#L55
There was a problem hiding this comment.
likewise, I care less about this logic being pulled in from grpc-common, and care more that we make sure there's one test that exercise the retry logic.
bcoe
left a comment
There was a problem hiding this comment.
I support @JustinBeckwith's approach of simply pulling in the logic from common-grpc, rather than undertaking a more extensive refactor in this PR; the benefit being that we more towards it being easier to pin @grpc/grpc-js.
I do think it would be beneficial to refactor this logic out of bigtable, it feels like it's logic that probably shouldn't be in this layer.
I've created a ticket tracking this follow up work: #556
src/index.ts
Outdated
| noResponseRetries: 0, | ||
| objectMode: true, | ||
| shouldRetryFn: (Service as any).shouldRetryRequest_, | ||
| shouldRetryFn: r => [429, 500, 502, 503].indexOf(r.code) > -1, |
There was a problem hiding this comment.
likewise, I care less about this logic being pulled in from grpc-common, and care more that we make sure there's one test that exercise the retry logic.
This is more a conversation starter than anything. The
@google-cloud/common-grpcmodule creates a few problems for us:google-gax,@google-cloud/common, and@google-cloud/common-grpcall referencegoogle-auth-library, which is causing diamond dependency issues when we update thingsThere is some code copying in here in
decorateStatusI don't feel great about. Hoping @alexander-fenster will have suggestions on if/how/where in gax this could live.With minimal effort, we could also drop the dependency on
@google-cloud/common. The error util libraries seem to be the only bits we're using. We should have a single set of Error classes we're using, and have those potentially available in their own shared single use library.