feat: multiplexed session as default session mode#2451
Conversation
3bdd009 to
e3ab09e
Compare
242bb79 to
ffc787e
Compare
|
Warning: This pull request is touching the following templated files:
|
e28ae67 to
eb48211
Compare
eb48211 to
d5411fb
Compare
| } | ||
| }, | ||
| ); | ||
| // awaiting 10ms for begin call to finish its execution |
There was a problem hiding this comment.
since we are not awaiting on begin in createReadStream(known issue) to finish its execution, which was resulting in beginTxnRequest to be undefined. hence having this 10ms of sleep
src/multiplexed-session.ts
Outdated
| */ | ||
| async _acquire(): Promise<Session | null> { | ||
| const span = getActiveOrNoopSpan(); | ||
| span.addEvent('Acquiring multiplexed session'); |
There was a problem hiding this comment.
have added these events for multiplexed session similar to we had for regular sessions
There was a problem hiding this comment.
nit: I feel these events are not required. For regular session it was required as there was a wait if the session is not available. We already have few events in _getSession for mux sessions
d5411fb to
82ff2b5
Compare
src/multiplexed-session.ts
Outdated
| */ | ||
| async _acquire(): Promise<Session | null> { | ||
| const span = getActiveOrNoopSpan(); | ||
| span.addEvent('Acquiring multiplexed session'); |
There was a problem hiding this comment.
nit: I feel these events are not required. For regular session it was required as there was a wait if the session is not available. We already have few events in _getSession for mux sessions
| const error = new Error('err'); | ||
|
|
||
| const sessionFactory = new SessionFactory(database, NAME); | ||
| describe('when multiplexed session is disabled', () => { |
There was a problem hiding this comment.
IIRC, we decided to remove these tests to remove the complexity.
There was a problem hiding this comment.
We've refactored the tests to focus on the default multiplexed mode and removed the complex env enable/disable permutation loops.
For the regular session-specific tests, we can just run them with GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS disabled. Since we aren't running the full matrix of env permutations anymore. IMO we can retain this test. WDYT?
There was a problem hiding this comment.
Sure. Make sure to verify if we are reverting the ENVs in after block
There was a problem hiding this comment.
yeah, that I have made sure!
82ff2b5 to
635e6c7
Compare
This PR contains code changes for making multiplexed session as default enabled session mode.
To disable the multiplexed session:
For RO Transactions -
GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS = 'false'For Partitioned DML -
For RW Transactions -
Additionally, this PR includes code changes in the kokoro pipeline to run the integration test against regular sessions.