fix: avoid unbalanced session pool creation by olavloite · Pull Request #2442 · googleapis/java-spanner (original) (raw)

This comment was indeed outdated. It was initially written before adding the isUnbalanced(..) part to the solution, as I initially thought that it would be enough to only randomize the order when a session was initially added to the pool. That was not enough, because it could easily happen that a session would be used multiple times before it would actually be released into the pool (that is; because there was always a waiter for a session, which meant that the session was given to that waiter instead of being released to the pool). In addition, it could still be that an initial randomization would give an unbalanced pool.

I've updated the comment to reflect the current situation.

Due to the condition !sessions.isEmpty(), apart from the first session, all other sessions will enter the else block and get added to the front of the queue. If this is intended, then isn't this comment inconsistent?

Hmmm.... I'm not sure I understand your question correctly. The !sessions.isEmpty() ensures that only the first session to be released into the pool enters the else block. We do that because adding it at a random place in an empty list does not make sense.

How are we measuring if a session was added for the first time or has been added before. Shouldn't that be a different state variable that get's toggled the first time the session was added to the pool?

The original implementation would only set it to RANDOM when the session was created, and then reset it to FIRST here. So in that implementation, it was a valid indicator for whether it was the first time it was added. With the addition of checking for an unbalanced pool, that has changed (but the comment unfortunately not), and a session can be added at a random position multiple times, but the value is always reset to FIRST after it has been added, as that is the default as long as the pool remains balanced.