fix: fix retry logic in mutate and read (#980)

* fix: fix retry logic in mutate * fix format * add todos * fix the same issue in read
googleapis · Jan 12, 2022 · bfa84e1 · bfa84e1
1 parent b058d3b
commit bfa84e1
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 7 deletions.
diff --git a/src/table.ts b/src/table.ts
@@ -905,8 +905,6 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
 
       activeRequestStream = requestStream!;
 
-      requestStream!.on('request', () => numRequestsMade++);
-
       const toRowStream = new Transform({
         transform: (rowData, _, next) => {
           if (
@@ -945,6 +943,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
         }
       });
       rowStream.pipe(userStream);
+      numRequestsMade++;
     };
 
     makeNewRequest();
@@ -1502,6 +1501,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
     const onBatchResponse = (
       err: ServiceError | PartialFailureError | null
     ) => {
+      // TODO: enable retries when the entire RPC fails
       if (err) {
         // The error happened before a request was even made, don't retry.
         callback(err);
@@ -1545,8 +1545,9 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
           gaxOpts: options.gaxOptions,
           retryOpts,
         })
-        .on('request', () => numRequestsMade++)
         .on('error', (err: ServiceError) => {
+          // TODO: this check doesn't actually do anything, onBatchResponse
+          // currently doesn't retry RPC errors, only entry failures
           if (numRequestsMade === 0) {
             callback(err); // Likely a "projectId not detected" error.
             return;
@@ -1575,6 +1576,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
           });
         })
         .on('end', onBatchResponse);
+      numRequestsMade++;
     };
 
     makeNextBatchRequest();

diff --git a/test/table.ts b/test/table.ts
@@ -1187,7 +1187,6 @@ describe('Bigtable/Table', () => {
           (stream as any).abort = () => {};
 
           setImmediate(() => {
-            stream.emit('request');
             // eslint-disable-next-line @typescript-eslint/no-explicit-any
             (emitters!.shift() as any)(stream);
           });
@@ -2372,7 +2371,6 @@ describe('Bigtable/Table', () => {
             });
 
             setImmediate(() => {
-              stream.emit('request');
               stream.emit('error', error);
             });
 
@@ -2471,7 +2469,6 @@ describe('Bigtable/Table', () => {
           });
 
           setImmediate(() => {
-            stream.emit('request');
             stream.end({entries: fakeStatuses});
           });
 
@@ -2526,7 +2523,6 @@ describe('Bigtable/Table', () => {
           });
 
           setImmediate(() => {
-            stream.emit('request');
             stream.end({entries: fakeStatuses.shift()});
           });