feat(spanner): add support for snapshot isolation - #2245
Conversation
7b2f36b to
854c1ee
Compare
854c1ee to
0913aec
Compare
0913aec to
d574252
Compare
c73a9b9 to
1a2b964
Compare
1a2b964 to
780461d
Compare
| /** | ||
| * Use option isolationLevel to add the isolation level in the transaction. | ||
| */ | ||
| setIsolationLevel(isolationLevel: any): void { |
There was a problem hiding this comment.
Don't use any here. Use protos.google.spanner.v1.TransactionOptions.IsolationLevel
There was a problem hiding this comment.
updated, thanks for the suggestion!
There was a problem hiding this comment.
Is this method used anywhere now ?
| } | ||
|
|
||
| export interface WriteAtLeastOnceOptions extends CallOptions { | ||
| defaultTransactionOptions?: Pick<RunTransactionOptions, 'isolationLevel'>; |
There was a problem hiding this comment.
why didn't we have change_stream here, it creates a RW transaction
There was a problem hiding this comment.
will add that option in a separate PR, it got skipped earlier
| if (options.isolationLevel) { | ||
| transaction!.setIsolationLevel(options.isolationLevel); | ||
| } else if (defaultTransactionOptions) { | ||
| transaction!.setIsolationLevel( | ||
| defaultTransactionOptions.isolationLevel | ||
| ); | ||
| } |
There was a problem hiding this comment.
This code along with below code is getting repeated. Create a method in transaction.ts as setReadWriteTransactionOptions and move the code there
if (options.optimisticLock) {
transaction!.useOptimisticLock();
}
if (options.excludeTxnFromChangeStreams) {
transaction!.excludeTxnFromChangeStreams();
}```
| interface MutateRowsOptions extends CommitOptions { | ||
| requestOptions?: Omit<IRequestOptions, 'requestTag'>; | ||
| excludeTxnFromChangeStreams?: boolean; | ||
| defaultTransactionOptions?: Pick< |
There was a problem hiding this comment.
This is a breaking change
850b7ae to
780461d
Compare
5ee608e to
68edb46
Compare
68edb46 to
8633c2c
Compare
| transaction!.excludeTxnFromChangeStreams(); | ||
| } | ||
| transaction?.setReadWriteTransactionOptions( | ||
| options && Object.keys(options).length |
There was a problem hiding this comment.
How would this handle the case, when TransactionOptions only has excludeTxnFromChangeStreams set and isolation_level needs to be taken from defaultTransactionOptions
| import {Database} from './database'; | ||
| import {google} from '../protos/protos'; | ||
| import IRequestOptions = google.spanner.v1.IRequestOptions; | ||
| import {protos} from '.'; |
There was a problem hiding this comment.
import {google} from '../protos/protos' is already imported.
You could import IsolationLevel on top like IRequestOptions
import IsolationLevel = google.spanner.v1.TransactionOptions.IsolationLevel
There was a problem hiding this comment.
done, thanks for the suggestion!
| requestOptions?: Pick<IRequestOptions, 'transactionTag'>; | ||
| optimisticLock?: boolean; | ||
| excludeTxnFromChangeStreams?: boolean; | ||
| isolationLevel?: protos.google.spanner.v1.TransactionOptions.IsolationLevel; |
There was a problem hiding this comment.
Use the IsolationLevel import instead of the full syntax.
| const defaults = { | ||
| timeout: 3600000, | ||
| isolationLevel: | ||
| protos.google.spanner.v1.TransactionOptions.IsolationLevel |
There was a problem hiding this comment.
Change it here and all other places.
| transaction!.setReadWriteTransactionOptions( | ||
| this.options && Object.keys(this.options).length | ||
| ? this.options | ||
| : this.session.parent._getSpanner().defaultTransactionOptions |
There was a problem hiding this comment.
Do not use _getSpanner() private method . Instead create a private method _getSpanner in this class and use it. Refer this method in all other classes
There was a problem hiding this comment.
have refactored the code, please check
|
|
||
| this._queuedMutations = []; | ||
| this._options = {readWrite: options}; | ||
| this._options.isolationLevel = |
There was a problem hiding this comment.
Similarly change the import and protos full path
| /** | ||
| * Set isolation level . | ||
| */ | ||
| if (options?.isolationLevel) { |
There was a problem hiding this comment.
how would this use the default isolation level if available ?
There was a problem hiding this comment.
I have updated the code
in the Transaction class constructor we are assigning the default isolationLevel value as this._options.isolationLevel = IsolationLevel.ISOLATION_LEVEL_UNSPECIFIED;
now here in this case this._options.isolationLevel = options?.isolationLevel? options?.isolationLevel:this._getSpanner().defaultTransactionOptions.isolationLevel;
if the isolationLevel will get pass at the transaction level it will assign that value to the this._options.isolationLevel otherwise this value this._getSpanner().defaultTransactionOptions.isolationLevel will get assign which if being passed from spanner options will use that value otherwise it will also be containing default value of unspecified
| /** | ||
| * Use option isolationLevel to add the isolation level in the transaction. | ||
| */ | ||
| setIsolationLevel(isolationLevel: any): void { |
There was a problem hiding this comment.
Is this method used anywhere now ?
| assert.strictEqual(options, fakeOptions); | ||
| }); | ||
|
|
||
| it('should optionally accept `option` isolationLevel when passed with Spanner options', () => { |
There was a problem hiding this comment.
Add test cases for below
-
Should override
isolationLevelfrom runTransactionOptions, when passed in bothdefaultTransactionOptionsandrunTransactionOptions -
Should use isolation level from
defaultTransactionOptionswhenrunTransactionOptionsare passed but does not consists of isolation level and contains other properties. The resulting options should be a combination ofdefaultTransactionOptionsandrunTransactionOptions
There was a problem hiding this comment.
- If
defaultTransactionOptionshas more properties except, isolation level, then other properties should be ignored.
There was a problem hiding this comment.
have added the below tests in spanner.ts
- 'should be able to use isolationLevel from Spanner Option when other options are passed at transaction level'
- 'should override isolationLevel from Spanner Option when passed at transaction level'
56cfbe1 to
b628ba6
Compare
8cd74dc to
ae96078
Compare
ae96078 to
aaaf522
Compare
| span.addEvent('Using Session', {'session.id': session?.id}); | ||
| this._releaseOnEnd(session!, transaction!, span); | ||
| try { | ||
| transaction!.setReadWriteTransactionOptions(options); |
There was a problem hiding this comment.
| transaction!.setReadWriteTransactionOptions(options); | |
| transaction!.setReadWriteTransactionOptions(options as RunTransactionOptions); |
There was a problem hiding this comment.
Do this for all other places
aaaf522 to
440f020
Compare
This PR contains code changes to add support for option IsolationLevel at the client level and at the transaction level. supported methods are(RW and Blind Write): ``` - writeAtLeastOnce - runTransactionAsync - runTransaction - getTransaction - async getTransaction(from transaction runner class) ```
* feat(spanner): support for Multiplexed Session Partitioned Ops * feat(spanner): add support for snapshot isolation (#2245) This PR contains code changes to add support for option IsolationLevel at the client level and at the transaction level. supported methods are(RW and Blind Write): ``` - writeAtLeastOnce - runTransactionAsync - runTransaction - getTransaction - async getTransaction(from transaction runner class) ``` * refactor * refactor
* feat(spanner): support for Multiplexed Session Partitioned Ops * feat(spanner): add support for snapshot isolation (#2245) This PR contains code changes to add support for option IsolationLevel at the client level and at the transaction level. supported methods are(RW and Blind Write): ``` - writeAtLeastOnce - runTransactionAsync - runTransaction - getTransaction - async getTransaction(from transaction runner class) ``` * refactor * refactor
This PR contains code changes to add support for option IsolationLevel at the client level and at the transaction level.
supported methods are(RW and Blind Write):