fix: Remove custom readrows retry logic and rely on gax for retries (#1422)

* getRanges

* Slight refactor of createReadStream

* Add header to table utils

* Refactor range and keys getting

* Pull request opts into a separate function

* Revert "Pull request opts into a separate function"

This reverts commit c2f4dfe.

* logical separation of ranges and keys

* Revert "Revert "Pull request opts into a separate function""

This reverts commit 4817863.

* set up the test frame

* Test is set up to evaluate streaming behavior

* Modify test with the mock server to pass first tst

* Fix tests that are appending startKey and endKey

* Getting all the tests working

- Transform the rowsLimit parameter into an integer
- Change the hook into a before hook so that we don’t attempt to create multiple mock servers
- Create a guard so that the stream only writes if there are row keys to write

* eliminate old createreadstream test

define new interfaces too

* Remove only. rowsLimit should be optional

* Make rowKeysRead type more specific

* Add after hook to shut down server

* Add the less than or equal to fn to utils

* remove server start

* Add splice ranges back to diagnose problem

* Revert "Add splice ranges back to diagnose problem"

This reverts commit 5edaf82.

* get all tests passing

readRowsReqOpts should have an ECMAscript prefix to completely hide it from the user. Also remove a useless  Filter.parse.

* Use tableUtils lessthanorequalto

* refactor: Move retries finish making createreadstream smaller

* More specific type for options

* Import type and replace with any

* Turn on gax streaming retries

* Try out the shouldRetryFn

* Service error or Google error for this function

Also uncommented retry logic to see how the test would behave.

* First PR correction - remove any

* Get rid of another any

* Get rid of the any suppressed by typescript

It was necessary to add guards to make this work. Also changed the error type to Google error.

* Should use CreateReadStreamRequest as the type

* Remove the comment disabling linting on any

* Remove the import. It is not used anymore.

* Remove the custom retry logic

* Wrote a test that mocks gapic that will compile

* Refactor the test so that it is easy to

Move the check out into a separate function.

* Add expected values for request

* Do not append retryRequestOptions to gax options

Appending retryRequestOptions will not work for any call that is also passing in the retry gax parameter.

* Factor out the retry options logic

Move functions out of table.ts that other methods should not be concerned with. Export the function that creates retry logic from this new module

* Rename the file to retry-options

This is going to be used by the mutate row retry options so the filename should not be so specific.

* Initial attempt at adding resumption logic

* skip

* Comment out check in test

* Keep the resumption strategy in table.ts for now

There is currently a proxyquire issue preventing us from moving code out of table.ts so make a TODO and move it later.

* Get proxyquire logic right

Move the resumption strategy back to its own file and get the proxyquire configs right so that tests pass.

* Remove unused code

* Wrap with arrow functions.

We can’t pass class methods in

* Use the resumption request

* Set up a unit test framework

This ensures the right parameters are passed along to the gapic layer.

* Make sure to pass along maxRetries to the gapic

* Do an empty write to ensure server called only one

* Gets all 10 tests passing

Some issue with the mock server is making it so that responses don’t get sent back the way they should. This change allows all the tests to pass.

* Pull testing code out into a dedicated object

* Mock out project id properly.

* Delete code

* maxRetries test rename

* Pass in the filter

* Remove unused parameter

* Create tableStrategyInfo case class

* Fix appProfileId issue

* Basic resumption test added

* More resumption tests

* Get resumption logic tests into its own module

* Modify test name with reumption strategy

* Skip a test

* Remove a function that isn’t used anymore

* inline building request options

* Remove unused imports

* Remove imports that are not used

* Inline some functions to eliminate confusion

* Move comment to where it was before

* Better test parameterization

* Replace grpc error codes with programmatic result

* Correct interface

* Uncomment some code removed from before

* Remove this TODO

* Move tests to unit test folder.

* Better comments

* Update comment

* better spacing

* correct grammar

* Change to retry options construction

* Add a test for passing custom retry options

* Fix the complex example passing options along

* Pull functions out of TableUtils

These functions do not need to exist as a static method. Put them in the readrows resumption module.

* Import bug fix

* Moved some tests over

Moved some unit tests over to the system test json file.

* Check to see if maxRetries is undefined

* Change retry options - wrap in gaxOpts

* Move over should remove ranges which were already read

* move more tests over

should not retry if limit is reached
should remove the keys which were already read
'should remove ranges which were already read

* Add limit test and retry if all keys are read test

* Move should not retry if all the ranges are read

* Expected error code is 4

* Move location of updates

* Correct tests with new info

* Correct the json test file

* Modify test to match old test

* Moved rststream test over

* Add framework to mock server so that error message

* Remove the RST stream test

* Delete retries test block

* Make sure to call canResume in all cases

* Remove retryOpts from mutate function

* indent so diff is easier to read

* Expand on comment

* Fix side effects from turning on streaming retries

* Simplify the diff

* Add comments for a better description

* Remove any and replace with more specific type

* Correct test types to CreateReadStreamRequest

* Coerce types and reintroduce parseInt

to match expectations

* Remove unused import

* Rename the comment as a TODO

* Complete comment

* Add latency measurements to mock server tests

This commit contains a tool useful for printing latency results.

* Revert "Add latency measurements to mock server tests"

This reverts commit d3d05a2.

* Revert "Complete comment"

This reverts commit 32fe3e5.

* Revert "Rename the comment as a TODO"

This reverts commit 80cf68c.

* Resolve the spacing issues

* Solve remaining spacing issues

* Add spaces back

* Add the new test back in

Add the test for the max retries 0 case.

* Change the Gapic layer back to what it was

* Fix max retries test

* Remove only

* Add header

* Add comments to the tester

* Add comments for the test functions.

* Remove the TODOs

* Fix lint warning

* Address another lint warning

* Remove Filter

* Add stronger type checking to userStream.end fn

* Create a better overloaded version of userStreamen

* Remove reference to proto

* Added a comment about streaming retries set to tru

* Add comments to determine what table.ts should do

* Update test/util/gapic-layer-tester.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

* Update test/util/gapic-layer-tester.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

* Update test/util/gapic-layer-tester.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

* Update system-test/read-rows.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

* For max retries have a default retry count

* Define a retry count constant

* Update the comment

* Move check for rst error. Rename MockGapicLayer.

* linter

* Make test case description more specific

* Rename the file

* RST stream error message inclusion

* Don’t retry on resource exhausted

* Remove service error from imports

* RetryInfo in the status details should retry

* Remove unnecessary code for compilation

* Update system-test/read-rows.ts

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>

* Remove the arrow function

* Remove comments and replace with constants

* Update the comment for getRowKeys

* Move variables inside the if statement

* Add a test case for row ranges and row keys

* Add a test case that doesn’t do any filtering

* Remove TODO

* remove type any

* Allow user to override codes

Also update the keys and ranges every time.

* Add another test for overriding the retry codes

* Make the test a little bit more robust

* Update the test framework to measure shouldRetry

* Add another test ensuring a retry doesn’t happen

* rename the test

* Make canResume always run unless shouldRetryFn  pr

* Gapic layer should expect empty retry codes

* Remove copied file

* Change the resumption strategy

* Replace 13 with enum

* Removed example

* npm run fix

* Revert "npm run fix"

This reverts commit dd42118.

* Remove overrides

* Remove irrelevant test

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Author: 3 people

src/index.ts

@@ -69,7 +69,6 @@ export interface RequestOptions {
     | 'BigtableTableAdminClient'
     | 'BigtableClient';
   reqOpts?: {};
-  retryOpts?: {};
   gaxOpts?: {};
   method?: string;
 }
@@ -445,6 +444,11 @@ export class Bigtable {
       {},
       baseOptions,
       {
+        // Setting gaxServerStreamingRetries to true ensures that for readrows,
+        // sampleRowKeys, mutateRows, generateInitialChangeStreamPartitions and
+        // readChangeStream calls in the data client that the new streaming
+        // retries functionality will be used.
+        gaxServerStreamingRetries: true,
         servicePath: customEndpointBaseUrl || defaultBaseUrl,
         'grpc.callInvocationTransformer': grpcGcp.gcpCallInvocationTransformer,
         'grpc.channelFactoryOverride': grpcGcp.gcpChannelFactoryOverride,
@@ -846,18 +850,6 @@ export class Bigtable {
     }
 
     function makeRequestStream() {
-      const retryRequestOptions = Object.assign(
-        {
-          currentRetryAttempt: 0,
-          noResponseRetries: 0,
-          objectMode: true,
-        },
-        config.retryOpts
-      );
-
-      config.gaxOpts = Object.assign(config.gaxOpts || {}, {
-        retryRequestOptions,
-      });
       prepareGaxRequest((err, requestFn) => {
         if (err) {
           stream.destroy(err);