Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove Begin transaction on non retryable error #2168

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

fix: Remove Begin transaction on non retryable error #2168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c62584c
chore: integration test fix
surbhigarg92 Jan 11, 2024
e7d385b
Merge branch 'googleapis:main' into main
surbhigarg92 Mar 4, 2024
e57b897
Merge branch 'googleapis:main' into main
surbhigarg92 Mar 4, 2024
c0c935e
Merge branch 'googleapis:main' into main
surbhigarg92 Mar 8, 2024
8875f2c
Merge branch 'googleapis:main' into main
surbhigarg92 Mar 26, 2024
a903eec
Merge branch 'googleapis:main' into main
surbhigarg92 Apr 9, 2024
50914dc
Merge branch 'googleapis:main' into main
surbhigarg92 Jun 17, 2024
95aa578
Merge branch 'googleapis:main' into main
surbhigarg92 Jun 26, 2024
738804b
Merge branch 'googleapis:main' into main
surbhigarg92 Jul 3, 2024
b53a82f
Merge branch 'googleapis:main' into main
surbhigarg92 Jul 31, 2024
eb858ac
Merge branch 'googleapis:main' into main
surbhigarg92 Aug 6, 2024
163699b
Merge branch 'googleapis:main' into main
surbhigarg92 Aug 8, 2024
0f6fda3
Merge branch 'googleapis:main' into main
surbhigarg92 Sep 23, 2024
4b173ba
Merge branch 'googleapis:main' into main
surbhigarg92 Sep 30, 2024
4261105
Merge branch 'googleapis:main' into main
surbhigarg92 Oct 10, 2024
6a77a21
Merge branch 'googleapis:main' into main
surbhigarg92 Oct 21, 2024
c597b74
Merge remote-tracking branch 'upstream/main'
surbhigarg92 Oct 21, 2024
56df6b4
Merge branch 'googleapis:main' into main
surbhigarg92 Oct 22, 2024
bd4647c
Merge branch 'googleapis:main' into main
surbhigarg92 Oct 24, 2024
f6fa667
feat: (observability): trace Database.runTransactionAsync
odeke-em Oct 21, 2024
f321749
fix: remove begin call for non retriable errors
surbhigarg92 Oct 21, 2024
4b98ec3
Merge branch 'main' into begin_on_non_retryable_error
alkatrivedi Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,21 +758,6 @@ export class Snapshot extends EventEmitter {
this._update(response.metadata!.transaction);
}
})
.on('error', err => {
setSpanError(span, err);
const wasAborted = isErrorAborted(err);
if (!this.id && this._useInRunner && !wasAborted) {
// TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170
this.begin();
} else {
if (wasAborted) {
span.addEvent('Stream broken. Not safe to retry', {
'transaction.id': this.id?.toString(),
});
}
}
span.end();
})
.on('end', err => {
if (err) {
setSpanError(span, err);
Expand Down Expand Up @@ -1349,22 +1334,6 @@ export class Snapshot extends EventEmitter {
this._update(response.metadata!.transaction);
}
})
.on('error', err => {
setSpanError(span, err as Error);
const wasAborted = isErrorAborted(err);
if (!this.id && this._useInRunner && !wasAborted) {
span.addEvent('Stream broken. Safe to retry');
// TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170
this.begin();
} else {
if (wasAborted) {
span.addEvent('Stream broken. Not safe to retry', {
'transaction.id': this.id?.toString(),
});
}
}
span.end();
})
.on('end', err => {
if (err) {
setSpanError(span, err as Error);
Expand Down
119 changes: 1 addition & 118 deletions test/spanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
});
Expand Down Expand Up @@ -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 = {
Expand Down
Loading