deleteRows transaction not atomic

[allenwang7]

Environment details

• OS: mac 10.13.6
• Node.js version: 8.11
• npm version: 5.6.0
• @google-cloud/spanner version: 2.0.0

Steps to reproduce

1. ???
2. ???

nodejs-spanner/src/transaction-request.js

Lines 271 to 301 in 2b8cdda

	deleteRows(table, keys, callback) {
	const mutation = {};
	mutation['delete'] = {
	table: table,
	keySet: {
	keys: arrify(keys).map(function(key) {
	return {
	values: arrify(key).map(codec.encode),
	};
	}),
	},
	};
	if (this.transaction) {
	this.queue_(mutation);
	return;
	}
	const reqOpts = {
	singleUseTransaction: {
	readWrite: {},
	},
	mutations: [mutation],
	};
	return this.request(
	{
	client: 'SpannerClient',
	method: 'commit',
	reqOpts: reqOpts,
	},
	callback
	);
	}

Performed that command up above. I tried to delete around 8000 rows, each having multiple indices/keys, so it went over the 20k mutations per transaction limit. I did not realize this, and ran it. The entire transaction should fail, but it actually deleted as much as it could under the 20k limit, then threw the over 20k mutations/transaction error. Is this expected?

Thanks!

[crwilcox]

@allenwang7, could you share the code fragment which caused this to happen? I imagine it resembles the sample code in the docstring, but I want to be sure I understand how this occurred.

[allenwang7]

@crwilcox , something like this.

const rows = [rows] // an array of rows
const keys = rows.map((row) => [row.id, row.shardId]);

// keys here have more than 20k mutations.
table.deleteRows(keys)
.catch((err) => console.log(err));

[stephenplusplus]

Would the fix here be to do some local validation and throw an error? Or is this a bug or enhancement upstream?

[callmehiphop]

/cc @snehashah16

[snehashah16]

Would the fix here be to do some local validation and throw an error? Or is this a bug or enhancement upstream?

There is no way to validate this in the client library.
That said, the backend should have thrown an error for exceeding mutations, and not committed any
change.

I notice the library is using the singleUse txn here. i would recommend we change it to an explicit begin txn -> buffer mutations -> commit workflow.

[stephenplusplus]

I'll close this, and we can let #347 serve as the tracker for implementing transaction usage throughout the library.

[snehashah16]

I recommend we open this bug again. This is also not a feature request.
Please keep this bug as a tracker to verify all issues are fixed after #347 is completed.