googleapis / googleapis/google-cloud-node
Transaction.commit and Transaction.createReadStream unconditionally retry by invoke Transaction.begin() if any non-ABORTED error is returned; really it should firstly check for error better
- Vorherrschende Sprache
- TypeScript
- Sterne
- 3.2k
- Forks
- 712
- Ø Merge
- 2 T. 9 Std.
- Gemergte PRs (30 T.)
- 104
Beschreibung
Discovered while implementing tracing for this package that we have this code https://github.com/googleapis/nodejs-spanner/blob/0342e74721a0684d8195a6299c3a634eefc2b522/src/transaction.ts#L1321-L1334
which when interpreted will always invoke Transaction.begin() as long this was the transaction was not previously run aka doesn't have an id and the error returned in .commit() was not ABORTED; but really if we just tried to insert data that already exists and the gRPC backend returns `6 ALREADY_EXISTS: Failed to insert row with primary key ({pk#SingerId:100}) due to previously existing row`
we shall ALWAYS try to invoke .begin() which then will again invoke .commit()
## Reproducing code
```javascript
'use strict';
const {
NodeTracerProvider,
TraceIdRatioBasedSampler,
} = require('@opentelemetry/sdk-trace-node');
const {BatchSpanProcessor} = require('@opentelemetry/sdk-trace-base');
const {
TraceExporter,
} = require('@google-cloud/opentelemetry-cloud-trace-exporter');
const exporter = new TraceExporter();
const provider = new NodeTracerProvider({
sampler: new TraceIdRatioBasedSampler(1.0),
});
provider.addSpanProcessor(new BatchSpanProcessor(exporter));
async function main(
projectId = 'my-project-id',
instanceId = 'my-instance-id',
databaseId = 'my-project-id'
) {
// Create the Cloud Spanner Client.
const {Spanner} = require('@google-cloud/spanner');
const spanner = new Spanner({
projectId: projectId,
observabilityOptions: {
tracerProvider: provider,
enableExtendedTracing: true,
},
});
const instance = spanner.instance(instanceId);
const database = instance.database(databaseId);
const update = {
sql: "INSERT INTO Singers(firstName, SingerId) VALUES('Foo', 100)",
};
database.runTransaction(async (err, transaction) => {
if (err) {
console.error(err);
return;
}
try {
await transaction.run(update);
} catch (err) {
console.error(err);
} finally {
await transaction.commit();
await new Promise(resolve => {
setTimeout(() => {
resolve();
}, 2000);
});
provider.forceFlush();
// This sleep gives ample time for the trace
// spans to be exported to Google Cloud Trace.
await new Promise(resolve => {
setTimeout(() => {
resolve();
}, 4000);
});
spanner.close();
}
});
}
process.on('unhandledRejection', err => {
console.error(err.message);
process.exitCode = 1;
});
main(...process.argv.slice(2));
```
and here is a trace of the problems to confirm visualization
## Currently wrong
## Expected
I'd classify as a P0 because this adds extra calls and latency to customer calls on the most minor inconvenience like just a typo or data existing
## Suggestion
We should only invoke Transaction.begin() on errors that are retryable for example a transient error or a timeout or server unknown internal errors, deadline_exceeded, status_unavailable, resource_exhaused
Kindly /cc-ing @surbhigarg92 @alkatrivedi
Beitragsleitfaden
Rechercherichtung
Beginne in src/transaction.ts bei den Zeilen 1321-1334 und verwende die bereitgestellte Reproduktion für Cloud Spanner mit einem ALREADY_EXISTS-Fehler, um das Retry-Verhalten zu beobachten. Verfolge, wie Transaction.commit und Transaction.createReadStream entscheiden, ob Transaction.begin() aufgerufen werden soll; abgeschlossen ist die Aufgabe, wenn nicht erneut versuchbare Fehler wie ALREADY_EXISTS keinen weiteren begin/commit-Versuch auslösen, während die angegebenen erneut versuchbaren Status dies weiterhin tun.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- node.js, typescript
- Bereich
- api, backend, databases
- Issue-Typ
- Bug
- Schwierigkeit
- 3/5
- Geschätzter Aufwand
- 1-2 Tage
- Aktivitätsstatus
- Veraltet
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 45/100