-
Notifications
You must be signed in to change notification settings - Fork 112
feat(spanner): add support for snapshot isolation #2245
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
Conversation
7b2f36b to
854c1ee
Compare
854c1ee to
0913aec
Compare
0913aec to
d574252
Compare
c73a9b9 to
1a2b964
Compare
1a2b964 to
780461d
Compare
src/transaction.ts
Outdated
| /** | ||
| * Use option isolationLevel to add the isolation level in the transaction. | ||
| */ | ||
| setIsolationLevel(isolationLevel: any): void { |
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.
Don't use any here. Use protos.google.spanner.v1.TransactionOptions.IsolationLevel
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.
updated, thanks for the suggestion!
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.
Is this method used anywhere now ?
src/database.ts
Outdated
| } | ||
|
|
||
| export interface WriteAtLeastOnceOptions extends CallOptions { | ||
| defaultTransactionOptions?: Pick<RunTransactionOptions, 'isolationLevel'>; |
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.
why didn't we have change_stream here, it creates a RW transaction
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.
will add that option in a separate PR, it got skipped earlier
src/database.ts
Outdated
| if (options.isolationLevel) { | ||
| transaction!.setIsolationLevel(options.isolationLevel); | ||
| } else if (defaultTransactionOptions) { | ||
| transaction!.setIsolationLevel( | ||
| defaultTransactionOptions.isolationLevel | ||
| ); | ||
| } |
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 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();
}```
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.
modified
src/table.ts
Outdated
| interface MutateRowsOptions extends CommitOptions { | ||
| requestOptions?: Omit<IRequestOptions, 'requestTag'>; | ||
| excludeTxnFromChangeStreams?: boolean; | ||
| defaultTransactionOptions?: Pick< |
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 is a breaking 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.
updated
850b7ae to
780461d
Compare
5ee608e to
68edb46
Compare
68edb46 to
8633c2c
Compare
src/database.ts
Outdated
| transaction!.excludeTxnFromChangeStreams(); | ||
| } | ||
| transaction?.setReadWriteTransactionOptions( | ||
| options && Object.keys(options).length |
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.
How would this handle the case, when TransactionOptions only has excludeTxnFromChangeStreams set and isolation_level needs to be taken from defaultTransactionOptions
src/transaction-runner.ts
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks for the suggestion!
src/transaction-runner.ts
Outdated
| requestOptions?: Pick<IRequestOptions, 'transactionTag'>; | ||
| optimisticLock?: boolean; | ||
| excludeTxnFromChangeStreams?: boolean; | ||
| isolationLevel?: protos.google.spanner.v1.TransactionOptions.IsolationLevel; |
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.
Use the IsolationLevel import instead of the full syntax.
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.
done
src/transaction-runner.ts
Outdated
| const defaults = { | ||
| timeout: 3600000, | ||
| isolationLevel: | ||
| protos.google.spanner.v1.TransactionOptions.IsolationLevel |
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.
Change it here and all other places.
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.
done
src/transaction-runner.ts
Outdated
| transaction!.setReadWriteTransactionOptions( | ||
| this.options && Object.keys(this.options).length | ||
| ? this.options | ||
| : this.session.parent._getSpanner().defaultTransactionOptions |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have refactored the code, please check
src/transaction.ts
Outdated
|
|
||
| this._queuedMutations = []; | ||
| this._options = {readWrite: options}; | ||
| this._options.isolationLevel = |
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.
Similarly change the import and protos full path
src/transaction.ts
Outdated
| /** | ||
| * Set isolation level . | ||
| */ | ||
| if (options?.isolationLevel) { |
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.
how would this use the default isolation level if available ?
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 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
src/transaction.ts
Outdated
| /** | ||
| * Use option isolationLevel to add the isolation level in the transaction. | ||
| */ | ||
| setIsolationLevel(isolationLevel: any): void { |
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.
Is this method used anywhere now ?
test/database.ts
Outdated
| assert.strictEqual(options, fakeOptions); | ||
| }); | ||
|
|
||
| it('should optionally accept `option` isolationLevel when passed with Spanner options', () => { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If
defaultTransactionOptionshas more properties except, isolation level, then other properties should be ignored.
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.
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
src/database.ts
Outdated
| span.addEvent('Using Session', {'session.id': session?.id}); | ||
| this._releaseOnEnd(session!, transaction!, span); | ||
| try { | ||
| transaction!.setReadWriteTransactionOptions(options); |
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.
| transaction!.setReadWriteTransactionOptions(options); | |
| transaction!.setReadWriteTransactionOptions(options as RunTransactionOptions); |
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.
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):