Conversation
| @@ -198,7 +198,7 @@ DatabaseId getDatabaseId() { | |||
| } | |||
| /** Create a single session. */ | |||
| SessionImpl createSession() { | |||
| SessionInstance createSession() { | |||
There was a problem hiding this comment.
@olavloite Presently I have not modified this for multiplexed session method. If you like the approach, I can do a bit more refactoring to ensure that the session creation flow (control plane) only uses the new SessionInstance class and we use SessionImpl on the data-plane side of things. Also I haven't modified the class docs as of now. Will do after we finalize the approach, naming conventions, etc.
| assertThat(spans.size()).isEqualTo(3); | ||
| if (useMultiplexed) { | ||
| assertThat(spans.size()).isEqualTo(4); |
There was a problem hiding this comment.
I'm noticing a flaky behavior due to the new assertions for multiplexed sessions. This is because multiplexed session creation does not block on application startup. Probably we can plug some logic in existing setWaitForMinSessions method to also wait for multiplexed session creation?
To start with this could be used for tests. @olavloite WDYT? This also goes back to our previous discussion that it's ok to block application startup if its an option that customers opt-into and is not switched on by default.
| } | ||
| @Override | ||
| public MultiplexedSessionFuture get() { | ||
| try { | ||
| return this.multiplexedSessionSettableApiFuture.get(); | ||
| // Creating a new reference where the request's span state can be stored. | ||
| SessionImpl sessionImpl = |
There was a problem hiding this comment.
The result of this block of code should be cached in a field in the SessionFutureWrapper object, so multiple calls to get() return the same instance every time.
(We probably don't call it multiple times in our current code, although I'm not 100% sure, but as this method implements Future#get(), we should adhere to the contract of that method.)
| * Spanner database. Sessions are managed internally by the client library, and users need not be | ||
| * aware of the actual session management, pooling and handling. | ||
| */ | ||
| class SessionInstance { |
There was a problem hiding this comment.
Maybe SessionReference? As in: This class is just a reference to a backend session.
| settableFuture.set(multiplexedSession); | ||
| MultiplexedSessionFuture multiplexedSessionFuture = | ||
| new MultiplexedSessionFuture(settableFuture, span); | ||
| if (multiplexedSessionFuture != null) { |
There was a problem hiding this comment.
nit:
- The field
multiplexedSessionFutureshould bevolatile. - The lazy initialization should use a lock (the lock only needs to be taken during the actual initialization)
See this for a concrete example: https://www.baeldung.com/java-singleton-double-checked-locking#implementation
SessionImplclass today has two responsibilities - a) POJO for holding session metadata and b) holding the current transaction context (transaction ID, span, etc.) . Given it has the dual responsibility, it is used both in control plane (during session creation) as well as data-plane (transaction execution).This approach used to work for regular sessions since regular sessions only had 1 transaction running concurrently. For multiplexed sessions, since there can be multiple transactions running per
SessionImplobject, it can no longer hold the transaction context.This PR does the refactoring in two steps -
SessionInstancewhich is a stateless POJO representing 1 session. This does not hold any transaction context. Refactor all member variables which is specific to a session (session name, databaseID, etc.) fromSessionImpltoSessionInstance.AtomicReference<SettableApiFuture<SessionInstance>>. There is only object for this stored as a global variable. This is only replaced when a new session gets created by maintainer.MultiplexedSessionFutureWrapper,MultiplexedSessionFutureandMultiplexedSession). Finally, we invokemarkBusyto set the span on theSessionImplobject. This works since we now have 1 reference ofSessionImplper new requestAlternate Approach - An alternate approach was explored at - #3016 where I was refactoring the
setSpan()method and removing the method fromSessionTransactioninterface. This approach somewhat worked for RO transactions but it will be difficult to extend it for RW transactions in future. Also the approach involves a lot of refactoring/testing for all other classes (AbstractReadContext,AsyncTransactionManagerImpl,TransactionRunnerImpl) that implementSessionTransactioninterface. So, it's best to not go by this approach to minimize the changes on the client library side.Testing - Parameterized the
SpanTestclass and tested that this refactoring works for Multiplexed sessions. Also locally tested multiplexed session via integration tests, performance tests.