Skip to content

Commit f321749

Browse files
surbhigarg92surbhigarg92
committed
fix: remove begin call for non retriable errors
1 parent f6fa667 commit f321749

File tree

5 files changed

+4
-152
lines changed

5 files changed

+4
-152
lines changed

‎observability-test/database.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1696,7 +1696,7 @@ describe('Database', () => {
16961696
'Unexpected span status message'
16971697
);
16981698

1699-
const expectedEventNames = ['Using Session'];
1699+
const expectedEventNames = ['Using Session', 'exception'];
17001700
assert.deepStrictEqual(
17011701
actualEventNames,
17021702
expectedEventNames,

‎observability-test/spanner.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ describe('EndToEnd', async () => {
488488
database.formattedName_
489489
);
490490
await database.runTransactionAsync(async transaction => {
491-
const [rows] = await transaction!.run('SELECT 1');
491+
await transaction!.run('SELECT 1');
492492
});
493493

494494
traceExporter.forceFlush();

‎src/database.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3385,7 +3385,7 @@ class Database extends common.GrpcServiceObject {
33853385
try {
33863386
return await runner.run();
33873387
} catch (e) {
3388-
setSpanError(span, e as Error);
3388+
setSpanErrorAndException(span, e as Error);
33893389
throw e;
33903390
} finally {
33913391
span.end();

‎src/transaction.ts‎

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -758,21 +758,6 @@ export class Snapshot extends EventEmitter {
758758
this._update(response.metadata!.transaction);
759759
}
760760
})
761-
.on('error', err => {
762-
setSpanError(span, err);
763-
const wasAborted = isErrorAborted(err);
764-
if (!this.id && this._useInRunner && !wasAborted) {
765-
// TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170
766-
this.begin();
767-
} else {
768-
if (wasAborted) {
769-
span.addEvent('Stream broken. Not safe to retry', {
770-
'transaction.id': this.id?.toString(),
771-
});
772-
}
773-
}
774-
span.end();
775-
})
776761
.on('end', err => {
777762
if (err) {
778763
setSpanError(span, err);
@@ -1349,22 +1334,6 @@ export class Snapshot extends EventEmitter {
13491334
this._update(response.metadata!.transaction);
13501335
}
13511336
})
1352-
.on('error', err => {
1353-
setSpanError(span, err as Error);
1354-
const wasAborted = isErrorAborted(err);
1355-
if (!this.id && this._useInRunner && !wasAborted) {
1356-
span.addEvent('Stream broken. Safe to retry');
1357-
// TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170
1358-
this.begin();
1359-
} else {
1360-
if (wasAborted) {
1361-
span.addEvent('Stream broken. Not safe to retry', {
1362-
'transaction.id': this.id?.toString(),
1363-
});
1364-
}
1365-
}
1366-
span.end();
1367-
})
13681337
.on('end', err => {
13691338
if (err) {
13701339
setSpanError(span, err as Error);

‎test/spanner.ts‎

Lines changed: 1 addition & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -452,11 +452,8 @@ describe('Spanner with mock server', () => {
452452
request.requestOptions!.transactionTag,
453453
'transaction-tag'
454454
);
455-
const beginTxnRequest = spannerMock.getRequests().find(val => {
456-
return (val as v1.BeginTransactionRequest).options?.readWrite;
457-
}) as v1.BeginTransactionRequest;
458455
assert.strictEqual(
459-
beginTxnRequest.options?.readWrite!.readLockMode,
456+
request.transaction?.begin?.readWrite?.readLockMode,
460457
'OPTIMISTIC'
461458
);
462459
});
@@ -3758,120 +3755,6 @@ describe('Spanner with mock server', () => {
37583755
);
37593756
});
37603757

3761-
it('should use beginTransaction on retry for unknown reason', async () => {
3762-
const database = newTestDatabase();
3763-
await database.runTransactionAsync(async tx => {
3764-
try {
3765-
await tx.runUpdate(invalidSql);
3766-
assert.fail('missing expected error');
3767-
} catch (e) {
3768-
assert.strictEqual(
3769-
(e as ServiceError).message,
3770-
`${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
3771-
);
3772-
}
3773-
await tx.run(selectSql);
3774-
await tx.commit();
3775-
});
3776-
await database.close();
3777-
3778-
const beginTxnRequest = spannerMock
3779-
.getRequests()
3780-
.filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
3781-
.map(req => req as v1.BeginTransactionRequest);
3782-
assert.deepStrictEqual(beginTxnRequest.length, 1);
3783-
});
3784-
3785-
it('should use beginTransaction on retry for unknown reason with excludeTxnFromChangeStreams', async () => {
3786-
const database = newTestDatabase();
3787-
await database.runTransactionAsync(
3788-
{
3789-
excludeTxnFromChangeStreams: true,
3790-
},
3791-
async tx => {
3792-
try {
3793-
await tx.runUpdate(invalidSql);
3794-
assert.fail('missing expected error');
3795-
} catch (e) {
3796-
assert.strictEqual(
3797-
(e as ServiceError).message,
3798-
`${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
3799-
);
3800-
}
3801-
await tx.run(selectSql);
3802-
await tx.commit();
3803-
}
3804-
);
3805-
await database.close();
3806-
3807-
const beginTxnRequest = spannerMock
3808-
.getRequests()
3809-
.filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
3810-
.map(req => req as v1.BeginTransactionRequest);
3811-
assert.deepStrictEqual(beginTxnRequest.length, 1);
3812-
assert.strictEqual(
3813-
beginTxnRequest[0].options?.excludeTxnFromChangeStreams,
3814-
true
3815-
);
3816-
});
3817-
3818-
it('should use beginTransaction for streaming on retry for unknown reason', async () => {
3819-
const database = newTestDatabase();
3820-
await database.runTransactionAsync(async tx => {
3821-
try {
3822-
await getRowCountFromStreamingSql(tx!, {sql: invalidSql});
3823-
assert.fail('missing expected error');
3824-
} catch (e) {
3825-
assert.strictEqual(
3826-
(e as ServiceError).message,
3827-
`${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
3828-
);
3829-
}
3830-
await tx.run(selectSql);
3831-
await tx.commit();
3832-
});
3833-
await database.close();
3834-
3835-
const beginTxnRequest = spannerMock
3836-
.getRequests()
3837-
.filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
3838-
.map(req => req as v1.BeginTransactionRequest);
3839-
assert.deepStrictEqual(beginTxnRequest.length, 1);
3840-
});
3841-
3842-
it('should use beginTransaction for streaming on retry for unknown reason with excludeTxnFromChangeStreams', async () => {
3843-
const database = newTestDatabase();
3844-
await database.runTransactionAsync(
3845-
{
3846-
excludeTxnFromChangeStreams: true,
3847-
},
3848-
async tx => {
3849-
try {
3850-
await getRowCountFromStreamingSql(tx!, {sql: invalidSql});
3851-
assert.fail('missing expected error');
3852-
} catch (e) {
3853-
assert.strictEqual(
3854-
(e as ServiceError).message,
3855-
`${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}`
3856-
);
3857-
}
3858-
await tx.run(selectSql);
3859-
await tx.commit();
3860-
}
3861-
);
3862-
await database.close();
3863-
3864-
const beginTxnRequest = spannerMock
3865-
.getRequests()
3866-
.filter(val => (val as v1.BeginTransactionRequest).options?.readWrite)
3867-
.map(req => req as v1.BeginTransactionRequest);
3868-
assert.deepStrictEqual(beginTxnRequest.length, 1);
3869-
assert.strictEqual(
3870-
beginTxnRequest[0].options?.excludeTxnFromChangeStreams,
3871-
true
3872-
);
3873-
});
3874-
38753758
it('should fail if beginTransaction fails', async () => {
38763759
const database = newTestDatabase();
38773760
const err = {

0 commit comments

Comments
 (0)