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

Aperta
#7,367 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
api: spanner library: spanner priority: p2 type: bug
Lingua principale
TypeScript
Stelle
3.2k
Fork
712
Merge medio
2g 9h
PR unite (30g)
104

Descrizione

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
Screenshot 2024-10-13 at 6 03 43 AM

## Expected
Screenshot 2024-10-13 at 6 04 23 AM

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

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Inizia in src/transaction.ts alle righe 1321-1334 e usa la riproduzione fornita di Cloud Spanner con un errore ALREADY_EXISTS per osservare il comportamento dei tentativi. Traccia il modo in cui Transaction.commit e Transaction.createReadStream decidono se chiamare Transaction.begin(); il lavoro è completato quando gli errori non ritentabili, come ALREADY_EXISTS, non attivano un altro tentativo di begin/commit, mentre gli stati ritentabili indicati continuano a farlo.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
node.js, typescript
Ambito
api, backend, databases
Tipo di issue
Bug
Difficoltà
3/5
Tempo stimato
1-2 giorni
Stato di attività
Ferma
Chiarezza
Specificata chiaramente
Idoneità per principianti
45/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.