Spanner: Add BatchCreateSessions rpc and use rotating channel number #6185

olavloite · 2019-08-28T14:42:53Z

Adds the BatchCreateSessions rpc to the Spanner client and uses this for session creation. A new SessionClient is added that encapsulates the details of how the session creation is evenly distributed over all available channels and that multiple calls might be necessary before all sessions have been created.
The sessions are returned by the SessionClient as an Enumeration that will yield results as soon as sessions are available. This way, the session pool does not have to wait until all the requested sessions have been created, but can start to serve sessions as soon as some have come available.

Updates #6169

snehashah16

I would do this implementation slightly differently

let SessionPool.initPool should call BatchCreateSessions for each channel..
lets also validate the requested sessions were returned or then restart request with the (requested - response) session count.
After all sessions are available, the pool should then be shuffled to scatter the sessions (so that not all sessions for a given channel are sequential in the list). This avoids passing the channel ID in all higher APIs. channel is a very a gRPC -layer concept and its best not exposed above that.

Additionally,

U may also need to refactor some code to replenish sessions if below minSessions (this can be optionally done by batch create sessions) and
delete unused sessions (to bring it <= minSessions) - which should be done from the tail of the list.

snehashah16 · 2019-08-28T19:11:53Z

...oud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java

  @Override
  public BatchReadOnlyTransaction batchReadOnlyTransaction(TimestampBound bound) {
-    SessionImpl session = spanner.createSession(db);
+    int channel = channelCounter.getAndIncrement() % spanner.getOptions().getNumChannels();


we should not expose the channel to such high-level methods.. these are meant to be light-weight..

snehashah16 · 2019-08-28T19:14:58Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java

+    Span span = tracer.spanBuilder(BATCH_CREATE_SESSIONS).startSpan();
+    try (Scope s = tracer.withSpan(span)) {
+      List<com.google.spanner.v1.Session> sessions =
+          gapicRpc.batchCreateSessions(


this may not return as many sessions as requested by sessionCount.

You should loop until all sessions are returned

snehashah16 · 2019-08-28T19:19:59Z

...oud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java

  @Override
  public BatchReadOnlyTransaction batchReadOnlyTransaction(BatchTransactionId batchTransactionId) {
-    SessionImpl session = spanner.sessionWithId(checkNotNull(batchTransactionId).getSessionId());
+    int channel = channelCounter.getAndIncrement() % spanner.getOptions().getNumChannels();


same as above

snehashah16 · 2019-08-28T19:20:56Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java

  }

-  SessionImpl sessionWithId(String name) {
+  List<SessionImpl> batchCreateSessions(final DatabaseId db, final int sessionCount, int channel)


curious where is this method called ?

codecov · 2019-08-31T19:34:45Z

Codecov Report

Merging #6185 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6185   +/-   ##
=========================================
  Coverage     47.11%   47.11%           
- Complexity    27378    27393   +15     
=========================================
  Files          2524     2524           
  Lines        277617   277617           
  Branches      31984    31979    -5     
=========================================
  Hits         130786   130786           
  Misses       137056   137056           
  Partials       9775     9775

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e31c3...16ed655. Read the comment docs.

olavloite · 2019-09-01T05:35:10Z

@snehashah16 The previous version of this PR only added the rpc definition for BatchCreateSessions (my intention was to break the change into somewhat smaller pieces), but I can see how that introduces more questions than convenience. I have now updated PR to include the changes to the session pool as well.

I also refactored the code quite extensively based on your suggestions. The details for spreading the calls over all channels is now encapsulated in the SessionClient class, along with making additional calls if the server does not return the number of sessions that was requested.

skuruppu

Thanks for promptly making the changes we discussed @olavloite. I've only managed to review SessionClient.java and SessionPool.java and I have a couple of minor comments. Hoping to review the rest later today.

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

skuruppu · 2019-09-12T04:27:47Z

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

+  }
+
+  /**
+   * Callback interface to be used for BatchCreateSessions. When sessions come available or session


"... sessions become available ..."

skuruppu · 2019-09-12T04:55:00Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

-          createSession();
+        int sessionCount = options.getMinSessions() - (totalSessions() + numSessionsBeingCreated);
+        if (sessionCount > 0) {
+          createSessions(sessionCount);


For all the calls to createSessions() aside from the one for initPool() I have a bit of a concern. For initPool(), we'll evenly distribute the sessions over the channels. But when sessions are replenished, in theory, we may be closing sessions in a given channel but then redistributing the replenished sessions across all channels. Over time, I can imagine that there will be an imbalance on the number of sessions in each channel. Especially if sessions are closed one at a time, then the new sessions to replace those will always end up in the first channel. I don't know if this is an issue in practice or how much more likely this is to happen with this implementation compared to the random distribution of sessions over channel.

@olavloite, do you have any thoughts on this?

The SessionClient keeps track of the last channel hint that was used for the previous sesson creation call, so also in situations where sessions are created one-by-one or in smaller batches, they will be distributed over all channels. It is also implemented in such a way that when the number of sessions that is to be created is not divisible by the number of channels, it will create 1 more session for the first remainder of the channels than for the rest. A couple of examples (I've translated the channel hint into the actual channel number in the examples for simplicity. The actual channel is the remainder of the division channelHint / numChannels):

Current channel hint is 0 and 1 session is created: The session is created for channel 0 and the channel hint is 1 after the creation.

Current channel hint is 1 and 3 sessions are created: The sessions are created for channels 1 to 3 and the channel hint is now 0.

Current channel hint is 0 and 2 sessions are created: The sessions are created for channels 0 and 1 and the channel hint is 2.

Current channel hint is 2 and 6 sessions are created: 2 sessions are created for channel 2 and 3 and 1 session is created for channel 0 and 1. The channel hint is now 2.

So the session creation is largely distributed as we would want.

Closing sessions is a little bit different, as it is a lot more random. Sessions are closed in the following situations:

The session pool contains more idle sessions than the MaxIdleSessions and MinSessions config values and the maintainer is running: The maintainer will delete (approx) 1/10 of the superfluous sessions. These are currently polled from the head of the session pool. (Note: This should be changed to the end of the session pool, as we have changed the order of the session pool to LIFO, and it is better to close session that have less recently been used.) The channel affiliation of these sessions is completely random.

Sessions that have been deemed to be invalid (i.e. have returned a 'Session not found' error) are also removed from the pool. The channel affiliation of these sessions are also completely random.

I.e. which sessions are closed, and which channels that are affiliated with those, is inherently random. That could in theory create an imbalance in the number of sessions per channel.

skuruppu · 2019-09-12T05:04:41Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+      numSessionsBeingCreated += sessionCount;
+      try {
+        // Create a batch of sessions. The actual session creation can be split into
+        // multiple gRPC calls and the sessions are returned by the enumerator as they come


"the sessions are returned by the enumerator" should be "the session consumer consumes the returned sessions as they become available".

skuruppu · 2019-09-12T05:13:43Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+          allSessions.add(pooledSession);
+          // Release the session to a random position in the pool to prevent that a
+          // batch of sessions that are affiliated with the same channel are all placed
+          // sequentially in the pool.


"to prevent the case that a batch of sessions that are affiliated ..."

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

...nts/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchCreateSessionsTest.java

skuruppu · 2019-09-12T10:55:27Z

...nts/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchCreateSessionsTest.java

+  }
+
+  @Test
+  public void testSpannerReturnsNoSessions() throws InterruptedException {


I don't quite understand the logic in this test. I think it's trying to test for the backend only returning all the sessions available to it (maxServerSessions) rather than returning no sessions. Did I misunderstand?

Yeah, you're right. The name is somewhat misleading. It tests what happens if the server first returns all the sessions available to it, and then stops returning any sessions (i.e. returns zero sessions and no error). I've renamed the test method.

skuruppu · 2019-09-12T11:10:01Z

...ents/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java

+          i
+              < Math.min(
+                  maxTotalSessions - sessions.size(),
+                  Math.min(maxNumSessionsInOneBatch, request.getSessionCount()));


For readability, it would be better if this for loop statement was changed to:

int maxSessionsToCreate = Math.min(maxNumSessionsInOneBatch, request.getSessionCount());
for (int i = 0; i < Math.min(maxTotalSessions - sessions.size(), maxSessionsToCreate); i++) { }

skuruppu · 2019-09-12T11:21:18Z

...d-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionClientTest.java

+          public void onSessionCreateFailure(Throwable t, int createFailureForSessionCount) {}
+        };
+    // We want 100 sessions, but each rpc will only return 5. We should still get 100 sessions from
+    // the enumerator.


Suggested change

// the enumerator.

// the consumer.

skuruppu · 2019-09-12T11:28:28Z

...oud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

+                      // runner.
+                      // The creation of the new transaction failed with a
+                      // SessionNotFoundException,
+                      // and the session was re-created before the run method was called.


Please fix the line wrapping here and above.

skuruppu · 2019-09-12T11:29:24Z

...oud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

+            // The rollback will also cause a SessionNotFoundException, but this is caught, logged
+            // and
+            // further ignored by the library, meaning that the session will not be re-created for
+            // retry. Hence rollback at call 1.


The line wrapping needs to be fixed here as well.

snehashah16

Have provided high level comments..

snehashah16 · 2019-09-13T18:28:08Z

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

+        } catch (Throwable t) {
+          consumer.onSessionCreateFailure(t, remainingSessionsToCreate);
+          break;
+        }


would we not retry ?
If retries are already handled by gax layer, could u verify what error codes are retried ?

Gax handles the retries, and will do so based on the settings here: https://github.com/googleapis/googleapis/blob/6b2ba2ae3124c22ecb56af7102c78110b8576671/google/spanner/v1/spanner_gapic.yaml#L93

That means that the method will retry on:

Server returns error code Unavailable

Deadline_Exceeded (client side) after 60 seconds, and it will give up retrying after 600 seconds.

snehashah16 · 2019-09-13T18:29:33Z

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

+              newSpannerException(ErrorCode.UNKNOWN, "Server did not return any sessions"),
+              remainingSessionsToCreate);
+          break;
+        }


when would this happen ?

Background context: we removed session limits and therefore this seems highly unlikely.

This happened once while stress testing the functionality on the staging environment, so I added this as a safeguard against an infinite loop. I have not seen this happen on the production environment.

i would still retry the request (same logic as numRequested > 0)

Exposing an unknown error is degradation/error to their subsequent read / write requests..

Changed to retry also when the number of returned sessions is 0.

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

snehashah16 · 2019-09-13T18:37:58Z

...cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java

+    synchronized (this) {
+      for (int i = 0; i < spanner.getOptions().getNumChannels(); i++) {
+        // Create one more session for the first X calls to fill up the remainder of the division.
+        int createCountForChannel = sessionCountPerChannel + (i < remainder ? 1 : 0);


i would simplify this

sessionCountPerChannel = sessionCount / spanner.getOptions().getNumChannels();
remainder = sessionCount % spanner.getOptions().getNumChannels();
// just add remainder to the requested count for last or first channel.
for ( channel i : channels) {
if ( i == 0 OR i == (numChannels - 1)) {
sessionCountRequested = sessionCountPerChannel + remainder
}
}

Evenly distributing is great but wont buy us much optimization. Simple code will definitely improve readability and avoid bugs.

Done.
This change also has the advantage that requests for numSessions < numChannels will be executed in one rpc call instead of numSessions rpc calls.

Thanks !!

it has a be a huge config mistake if customers have "numSessions < numChannels"
Though this change guards against using all those number of channels.

snehashah16 · 2019-09-13T18:39:59Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+          if (sessionReplacementCount > MAX_SESSION_REPLACES) {
+            throw SpannerExceptionFactory.newSpannerException(
+                e.getErrorCode(),
+                "Could not replace invalidated session because the maximum number of times the session may be replaced was exceeded",


This is not a good experience for our customers since this degrades their read / writes.

When would this happen ?

Sorry, this shouldn't have been in this PR and has been removed.

It could happen if a large number of sessions in the pool have been invalidated by the server without the client knowing about it, and this would safeguard against a large number of retries that all fail.

sessions in the pool are kept alive using "GetSession() rpc" or "SELECT 1" query.

Hopefully we shouldnt have this catastrophic event

snehashah16 · 2019-09-13T18:41:20Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

          }
        } catch (SessionNotFoundException e) {
+          sessionReplacementCount++;
+          if (sessionReplacementCount > MAX_SESSION_REPLACES) {


same as above.. just leave this comment open until there is a better strategy defined for prev instance

snehashah16 · 2019-09-13T18:41:29Z

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

            break;
          } catch (SessionNotFoundException e) {
+            sessionReplacementCount++;
+            if (sessionReplacementCount > MAX_SESSION_REPLACES) {


same as above.. just leave this comment open until there is a better strategy defined for prev instance

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

snehashah16 · 2019-09-16T17:39:08Z

@skuruppu - i have reviewed the crux of the CL (mostly SessionClient, SpannerImpl and SessionPool.java)
Feel free to LGTM when u are done with the review.

snehashah16 · 2019-09-16T17:42:47Z

@olavloite - LGTM! feel free to check-in when @skuruppu is done with the review.

QQ: When the LRU session is returned to the pool, when does the BeginTxn rpc get called ?
I am curious if session is only returned to the pool after a transaction is started OR does the BeginTxn rpc occur in the write request path ?

olavloite · 2019-09-18T08:01:33Z

@snehashah16

QQ: When the LRU session is returned to the pool, when does the BeginTxn rpc get called ?
I am curious if session is only returned to the pool after a transaction is started OR does the BeginTxn rpc occur in the write request path ?

When a session is released into the pool (be it a session that is returned from a client, a session that has just been created or a session that has just executed a keep-alive query), the session pool will check whether it needs to prepare a transaction or not. So what happens is:

A session is returned to the pool.
The session pool checks whether a read/write transaction should be prepared by checking this: currentWriteFraction < wantWriteFraction || numReadWriteWaiters > numSessionsBeingPrepared
If false --> Session is added to the read queue. If true --> the following steps are executed.
The session will not be added to the queue of ready read- or write sessions, but instead an asynchronous BeginTransaction rpc will be started using the same executor that is also used for creating sessions.
When the BeginTransaction rpc has finished, the session will be returned to the first read/write waiter (if any), and otherwise it will be added to the head of the read/write sessions queue.

So the BeginTransaction rpc is not executed as part of the write request path.

However, if the write fraction setting of the session pool is too low for the number of read/write transactions that is executed, the BeginTransaction rpc will be a bottleneck, as any read/write transaction that wants a session will be placed in the wait queue and will wait for a BeginTransaction rpc to finish.

The BeginTransaction rpc can also be a bottleneck in a very high load read/write transaction situation for another reason:

Preparing read/write transactions in the session pool is (by default) executed using a shared grpc-transport executor which is fixed to 8 worker threads.
If the client application is executing (short) read/write transactions using a thread pool with more than 8 worker threads, there is a chance that the executor that is used for preparing transactions will not be able to keep up with the client application.

olavloite requested review from kolea2 and skuruppu August 28, 2019 14:42

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 28, 2019

snehashah16 reviewed Aug 28, 2019

View reviewed changes

olavloite force-pushed the spanner-add-batch-create-sessions-rpc branch from 62f27b6 to f1bbdcb Compare August 31, 2019 19:26

olavloite requested a review from snehashah16 September 1, 2019 05:35

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2019

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 3, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 3, 2019

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2019

skuruppu requested a review from larkee September 5, 2019 05:17

skuruppu reviewed Sep 12, 2019

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 13, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 13, 2019

olavloite requested a review from skuruppu September 13, 2019 18:04

snehashah16 reviewed Sep 13, 2019

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2019

olavloite added 5 commits September 17, 2019 10:16

add BatchCreateSessions rpc and use rotating channel number

639dd68

add test for batchCreateSessions

6fd8ec7

use BatchCreateSessions

33fb1b9

use batchCreateSessions

7baa9ef

fix formatting

b7e8ceb

olavloite added 17 commits September 17, 2019 10:16

revert unnecessary changes

afaad0e

fix formatting

b3c149f

synchronize around increasing channel counter

08c79c6

remove printStackTrace()

06f05d8

do session creation in loop

490457a

take consumer instead of returning enumerator

ed593f3

add some documentation to session pool

5cc9a03

processed review comments

bb00a9e

unflake test case

0b6c784

fix potential infinite loop

170ee1c

unflake testSpannerReturnsAllAvailableSessionsAndThenNoSessions

fe15c27

prevent infinite loop in session retry

fb899d5

fail if exhausted

0ce840a

temp throw error to debug hanging test

088d654

remove test code

d8f20f3

processed review comments

2df414a

mock batchCreateSessions after rebase

d41e83c

olavloite force-pushed the spanner-add-batch-create-sessions-rpc branch from 36a02e6 to d41e83c Compare September 17, 2019 08:56

retry if the number of returned sessions is 0

16ed655

skuruppu approved these changes Sep 25, 2019

View reviewed changes

skuruppu merged commit 7c1172b into googleapis:master Sep 27, 2019

chingor13 mentioned this pull request Oct 2, 2019

Release google-cloud-java v0.112.0 #6420

Merged

codyoss mentioned this pull request Nov 6, 2019

Release google-cloud-java v0.112.0 #6703

Closed

Spanner: Add BatchCreateSessions rpc and use rotating channel number #6185

Spanner: Add BatchCreateSessions rpc and use rotating channel number #6185

Uh oh!

Conversation

olavloite commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snehashah16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olavloite commented Sep 1, 2019

Uh oh!

skuruppu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehashah16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olavloite commented Aug 28, 2019 •

edited

Loading

codecov bot commented Aug 31, 2019 •

edited

Loading