feat: Support for Spanner ReadLockMode

[purva9413]

Feature to add ReadLockMode option for ReadWrite transactions only. Two ways users can set the option -

1. Set it on the base transactionOption array like any other txn option. This provides consistency with how our other option configurations work
2. Set it directly on the readWrite options inside the transactionOption array. This provides flexibility for the user since the readLockMode is a special nested option inside of the readWrite.

Some comment changes to be in accordance with this new feature and unit tests have been added.

[purva9413]

Opening this up for review to get early feedback. The style failures seem to be for already existing code lines in the unit test class, so not sure if I need to do anything there. Will follow up on that.

[bshaffer]

This looks great!

As mentioned in the team meeting, I don't want to support the nested version of this option. We should only document and support one way to use it. If it's usable the other "nested" way, that's fine, but we don't need to test it and support it specifically.

Can you confirm that this option should NOT be supported in the other places TransactionOptions is used? This PR adds support for Database::runTransaction and Database::transaction, but #7749 shows that transaction options are used in the following methods as well:

• Database::executePartitionedUpdate
• Operation::transaction (this is called by Database::transaction)
• Operation::execute
• Operation::executeUpdate
• Database::batchUpdate

[bshaffer]

Let's only support it on the top level to keep it simple

Suggested change

	// The `readLockMode` can be set on the base options following convention or as a nested option if need be
	if (isset($options['readLockMode']) || isset($options['readWrite']['readLockMode'])) {
	$readLockModeOption = $options['readLockMode'] ?? $options['readWrite']['readLockMode'];
	$transactionOptions['readWrite']['readLockMode'] = $readLockModeOption;
	}
	// The `readLockMode` can be set on the base options following convention or as a nested option if need be
	if (isset($options['readLockMode'])) {
	$transactionOptions['readWrite']['readLockMode'] = $options['readLockMode']
	}

[purva9413]

I think we discussed keeping this officially documented as a option settable on the top level. But if someone wants to set it in the nested way they can? If we remove this condition, and someone tries to set it in the nested way it will not set the option at all as we are creating an empty readWrite array at the beginning of this function.

Let me know if that makes sense or we still want to let users only set this as a top level option (having that as the only choice might cause some confusion if folks go in and check the way the ReadLockMode is setup as a nested option in our compiled proto).

[bshaffer]

looks good! All small suggestions except for one regarding DX

[bshaffer]

why are we supplying 'readWrite' => [] here? Is it not sufficient to supply the readLockMode option? This seems like a weird API... supplying an empty array to readWrite does not make sense to me from a DX standpoint.

[purva9413]

That sounds reasonable, agreed it is probably not the best way to go about in terms of dev experience. I have removed the 'readWrite' => []. But in order to now format the readLockMode correctly and only for readWrite transactions, I have added a condition into Grpc::formatTransactionOptions(). Lmk if that looks good.

[bshaffer]

great work! This is still marked as "do not merge", but it's approved on my end.

[snippet-bot]

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment