refactor: move multiplexed session handling to separate class

[olavloite]

Refactors the multiplexed session implementation to use its own class instead of being combined with the existing session pool. This simplifies the code path for multiplexed sessions, and makes it possible to entirely remove the SessionPool class when all operations can be executed on multiplexed sessions.

[arpan14]

This will be a new thread which will maintain multiplexed sessions? If yes, can you share if re-using the existing thread was a harder implementation?

[olavloite]

It would have been slightly harder, but that was not my main reason for not using the same thread. My reasoning for creating a separate maintainer and executor was:

1. This separates multiplexed sessions completely from the session pool. That means that once all operations work on multiplexed sessions, we can just remove the entire session pool implementation.
2. This maintainer uses a ScheduledThreadPoolExecutor with zero core threads, and only runs the task once every 10 minutes. The additional resource usage is therefore minimal.

[arpan14]

Nit: The formatting looks off. Also, should we add a TODO that this is a temporary measure and we can remove it in future when this has a bit of increased usage?

[olavloite]

I added a TODO for removing.

I'm not sure what you mean with the formatting being off (?) The code is formatted using the normal formatter, and I don't see anything strange with this line or the ones directly above.

[arpan14]

Given we are initializing this to -1, can there be an edge case where the value gets decremented to -1? Would using a boxed integer and setting null be a better default than -1?

[olavloite]

I think that there's a bit of confusion between this value and the numCurrentSingleUseTransactions:

• The channel hint that is generated will never be -1. It is always calculated by getting the first clear bit from index 0. That means that it will always be >= 0.
• The numCurrentSingleUseTransactions gets incremented and decremented based on transactions being started and ended. That value should also never get to -1. In theory it could if we were 'over-closing' single-use transactions, but even then we have a safe-guard in the onReadDone() method that only decrements it the first time the method is called, and ignores any second call.

One other scenario that is a bit more probable (but still not much) is that reads never call onReadDone. This could cause numCurrentSingleUseTransactions to get higher than it should. The effect of this would be that we would fall back to using a random channel hint more often than we in the ideal case should. (The same method is also used to end spans, which means that it would also cause spans not to be ended. That is already the case in the current regular session implementation.)

[arpan14]

1. How is the channel hint managed for multi-use transactions?
2. Given we will always set the options at SessionImpl, the random hint logic that was introduced in AbstractReadContext becomes dead code. Do we clean it up here itself? Or do that in a separate PR? I'm fine either ways.

[olavloite]

1. Channel hint for multi-use read-only transactions are always random. In theory we could use the same strategy for them as for single-use read-only transactions, but I think we should not, because it would make the 'channel-hint-bitset' depend on read-only transactions always being closed. Failure to close read-only transactions has been a common source of session leaks in the past, and that is exactly what we want to get rid of with multiplexed sessions. Adding a new dependency on closing all read-only transactions here could in theory give us a bit better latency if someone is running 1qps multi-use read-only transactions, but I don't think the benefit of that outweighs the potential downside.
2. The code for assigning a random hint in AbstractReadContext is still used. We only create an options instance if the channel hint is not the special value NO_CHANNEL_HINT. See

   java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java

   Line 125 in 5979f2d

   if (channelHint == NO_CHANNEL_HINT) {

   . What happens in this case is the following:
   2.1. MultiplexedSessionDatabaseClient returns NO_CHANNEL_HINT if we should just use a random channel:

   java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClient.java

   Line 315 in 5979f2d

   return NO_CHANNEL_HINT;

   
   2.2. That value is then passed to the constructor of SessionImpl.
   2.3. SessionImpl checks if the channel hint is NO_CHANNEL_HINT, and if so does not create an options instance, but instead calls sessionReference.getOptions().
   2.4. sessionReference.getOptions() returns null for multiplexed sessions and the affiliated channel for regular sessions.

[psinghbay1]

👍