fix: remove begin call for non retriable errors

googleapis · Oct 21, 2024 · 65acc23 · 65acc23
1 parent 4d7b0aa
commit 65acc23
Show file tree

Hide file tree

Showing 5 changed files with 4 additions and 151 deletions.
diff --git a/observability-test/database.ts b/observability-test/database.ts
@@ -1687,7 +1687,7 @@ describe('Database', () => {
         'Unexpected span status message'
       );
 
-      const expectedEventNames = ['Using Session'];
+      const expectedEventNames = ['Using Session', 'exception'];
       assert.deepStrictEqual(
         actualEventNames,
         expectedEventNames,

diff --git a/observability-test/spanner.ts b/observability-test/spanner.ts
@@ -473,7 +473,7 @@ describe('EndToEnd', () => {
         database.formattedName_
       );
       await database.runTransactionAsync(async transaction => {
-        const [rows] = await transaction!.run('SELECT 1');
+        await transaction!.run('SELECT 1');
       });
 
       traceExporter.forceFlush();

diff --git a/src/database.ts b/src/database.ts
@@ -3395,7 +3395,7 @@ class Database extends common.GrpcServiceObject {
             try {
               return await runner.run();
             } catch (e) {
-              setSpanError(span, e as Error);
+              setSpanErrorAndException(span, e as Error);
               throw e;
             } finally {
               span.end();

diff --git a/src/transaction.ts b/src/transaction.ts
@@ -743,21 +743,6 @@ export class Snapshot extends EventEmitter {
             this._update(response.metadata!.transaction);
           }
         })
-        .on('error', err => {
-          const isServiceError =
-            err && typeof err === 'object' && 'code' in err;
-          if (
-            !this.id &&
-            this._useInRunner &&
-            !(
-              isServiceError &&
-              (err as grpc.ServiceError).code === grpc.status.ABORTED
-            )
-          ) {
-            this.begin();
-          }
-          setSpanError(span, err);
-        })
         .on('end', err => {
           if (err) {
             setSpanError(span, err);
@@ -1318,21 +1303,6 @@ export class Snapshot extends EventEmitter {
             this._update(response.metadata!.transaction);
           }
         })
-        .on('error', err => {
-          setSpanError(span, err as Error);
-          const isServiceError =
-            err && typeof err === 'object' && 'code' in err;
-          if (
-            !this.id &&
-            this._useInRunner &&
-            !(
-              isServiceError &&
-              (err as grpc.ServiceError).code === grpc.status.ABORTED
-            )
-          ) {
-            this.begin();
-          }
-        })
         .on('end', err => {
           if (err) {
             setSpanError(span, err as Error);

diff --git a/test/spanner.ts b/test/spanner.ts
@@ -452,11 +452,8 @@ describe('Spanner with mock server', () => {
         request.requestOptions!.transactionTag,
         'transaction-tag'
       );
-      const beginTxnRequest = spannerMock.getRequests().find(val => {
-        return (val as v1.BeginTransactionRequest).options?.readWrite;
-      }) as v1.BeginTransactionRequest;
       assert.strictEqual(
-        beginTxnRequest.options?.readWrite!.readLockMode,
+        request.transaction?.begin?.readWrite?.readLockMode,
         'OPTIMISTIC'
       );
     });
@@ -3758,120 +3755,6 @@ describe('Spanner with mock server', () => {
       );
     });
 
-    it('should use beginTransaction on retry for unknown reason', async () => {
-      const database = newTestDatabase();
-      await database.runTransactionAsync(async tx => {
-        try {
-          await tx.runUpdate(invalidSql);
-          assert.fail('missing expected error');
-        } catch (e) {
-          assert.strictEqual(
-            (e as ServiceError).message,
-            `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
-          );
-        }
-        await tx.run(selectSql);
-        await tx.commit();
-      });
-      await database.close();
-
-      const beginTxnRequest = spannerMock
-        .getRequests()
-        .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
-        .map(req => req as v1.BeginTransactionRequest);
-      assert.deepStrictEqual(beginTxnRequest.length, 1);
-    });
-
-    it('should use beginTransaction on retry for unknown reason with excludeTxnFromChangeStreams', async () => {
-      const database = newTestDatabase();
-      await database.runTransactionAsync(
-        {
-          excludeTxnFromChangeStreams: true,
-        },
-        async tx => {
-          try {
-            await tx.runUpdate(invalidSql);
-            assert.fail('missing expected error');
-          } catch (e) {
-            assert.strictEqual(
-              (e as ServiceError).message,
-              `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
-            );
-          }
-          await tx.run(selectSql);
-          await tx.commit();
-        }
-      );
-      await database.close();
-
-      const beginTxnRequest = spannerMock
-        .getRequests()
-        .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
-        .map(req => req as v1.BeginTransactionRequest);
-      assert.deepStrictEqual(beginTxnRequest.length, 1);
-      assert.strictEqual(
-        beginTxnRequest[0].options?.excludeTxnFromChangeStreams,
-        true
-      );
-    });
-
-    it('should use beginTransaction for streaming on retry for unknown reason', async () => {
-      const database = newTestDatabase();
-      await database.runTransactionAsync(async tx => {
-        try {
-          await getRowCountFromStreamingSql(tx!, {sql: invalidSql});
-          assert.fail('missing expected error');
-        } catch (e) {
-          assert.strictEqual(
-            (e as ServiceError).message,
-            `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
-          );
-        }
-        await tx.run(selectSql);
-        await tx.commit();
-      });
-      await database.close();
-
-      const beginTxnRequest = spannerMock
-        .getRequests()
-        .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
-        .map(req => req as v1.BeginTransactionRequest);
-      assert.deepStrictEqual(beginTxnRequest.length, 1);
-    });
-
-    it('should use beginTransaction for streaming on retry for unknown reason with excludeTxnFromChangeStreams', async () => {
-      const database = newTestDatabase();
-      await database.runTransactionAsync(
-        {
-          excludeTxnFromChangeStreams: true,
-        },
-        async tx => {
-          try {
-            await getRowCountFromStreamingSql(tx!, {sql: invalidSql});
-            assert.fail('missing expected error');
-          } catch (e) {
-            assert.strictEqual(
-              (e as ServiceError).message,
-              `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
-            );
-          }
-          await tx.run(selectSql);
-          await tx.commit();
-        }
-      );
-      await database.close();
-
-      const beginTxnRequest = spannerMock
-        .getRequests()
-        .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
-        .map(req => req as v1.BeginTransactionRequest);
-      assert.deepStrictEqual(beginTxnRequest.length, 1);
-      assert.strictEqual(
-        beginTxnRequest[0].options?.excludeTxnFromChangeStreams,
-        true
-      );
-    });
-
     it('should fail if beginTransaction fails', async () => {
       const database = newTestDatabase();
       const err = {