-
Notifications
You must be signed in to change notification settings - Fork 452
feat!: Spanner V2 #7841
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.
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
feat!: Spanner V2 #7841
Conversation
fd840b9 to
b01343c
Compare
4dc7761 to
a52f65c
Compare
473a099 to
2cb8167
Compare
57510f6 to
2c33f61
Compare
2cb8167 to
ea4cdcb
Compare
2cc1634 to
a324b05
Compare
691e07c to
82baba4
Compare
|
|
||
| // There isn't anything configurable here. | ||
| $options['transactionOptions'] = $this->configureTransactionOptions($options['transactionOptions'] ?? []); | ||
| $options['transactionOptions'] = $this->configureReadWriteTransactionOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unusure of the call to configure the txn options here. If the $options['transactionOptions'] is set, this is essentially a no-op, and if this is not set it just initializes the readWrite options on the transactionOptions. We could add in a method to just init readWrite options (I have a similar suggestion in TransactionConfigurationTrait.php) and then simplify this with something like
$options['transactionOptions'] = $options['transactionOptions'] ?? $this->initReadWriteTransaction();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also help untangle some of the calling stack as there is a similar call to configure here as well.
Happy to discuss this in more details, I think our unit tests should be able to catch issues if they come up with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've added calls using validateOptions to at least add structure to these functions.
db21d28 to
ed9d000
Compare
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
dd1e1ad to
05cdfb4
Compare
0bc1934 to
05cdfb4
Compare
69c9dcf to
b763b2b
Compare
c2bb1c3 to
eae013d
Compare
TODO
Verify thatmoved to chore(Spanner): TransactionOptionsBuilder refactor (WIP) #8653configureTransactionOptionsis either no longer being called twice inrunTransaction, or that it has a good reason for being called twice (cc @purva9413 )BREAKING_CHANGE_REASON=Spanner v2
BEGIN_VERSION_OVERRIDE
Core: 1.68.0
END_VERSION_OVERRIDE