spanner: Enhance session pool behavior by vkedia · Pull Request #2026 · googleapis/google-cloud-java

vkedia · 2017-05-01T20:44:44Z

Change defaults for maxSessions and blockIfExhauseted
Added tracking of leaked sessions
Changed closeAsync to close leaked sessions as well.
Changed closeAsync to close pool maintenance worker as well.
Added a SessionPoolStressTester
Changed Spanner#closeAsync to be blocking Spanner#close.

- Change defaults for maxSessions and blockIfExhauseted - Changed closeAsync to close leaked sessions as well. - Changed closeAsync to close pool maintenance worker as well. - Added a SessionPoolStressTester - Changed Spanner#closeAsync to be blocking Spanner#close.

vkedia · 2017-05-01T20:45:22Z

@tomayles Please take a look

coveralls · 2017-05-01T20:56:24Z

Coverage decreased (-0.006%) to 80.954% when pulling c9d56df on vkedia:session-pool-exception into 9623994 on GoogleCloudPlatform:master.

vkedia · 2017-05-01T21:40:55Z

Closed by mistake.

coveralls · 2017-05-01T21:52:11Z

Coverage decreased (-0.009%) to 80.952% when pulling ae8860f on vkedia:session-pool-exception into 9623994 on GoogleCloudPlatform:master.

danielcompton

I'm not so keen on Spanner changing from closeAsync to close. It is handy to be able to set a timeout when closing the Spanner to handle cases where it gets blocked for whatever reason. At the bottom, this ends up doing (blocking?) RPC calls to deleteSession. Perhaps this is totally safe, if so, it could be good to add a note about this in the implementation notes?

I'm not sure if this is throwing the kitchen sink in, but having close and closeAsync might be useful.

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

-   * #getReadWriteSession()} will start throwing {@code IllegalStateException}. The returned {@code
-   * ListenableFuture} blocks till all the sessions created in this pool have been closed.
+   * Close all the sessions. Once this method is invoked {@link #getReadSession()} and {@link
+   * #getReadWriteSession()} will start throwing {@code IllegalStateException}. This method blocks


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

   * logging and tracking of leaked sessions.
   */
-  ApiFuture<Void> closeAsync();
+  void close();


coveralls · 2017-05-01T22:28:03Z

Coverage decreased (-0.009%) to 80.952% when pulling 531f73c on vkedia:session-pool-exception into 9623994 on GoogleCloudPlatform:master.

danielcompton · 2017-05-01T22:47:06Z

Thanks for updating the javadocs. I'm still a little uneasy about having blocking RPC calls in the shutdown path though.

vkedia · 2017-05-01T23:02:45Z

Thanks for your comments. It is almost essential that this method be allowed to finish and delete all the sessions otherwise it can lead to session lying around on the backend for upto an hour. Since there is a [hard limit](https://cloud.google.com/spanner/docs/limits) on how many sessions one can have on the backend, this can lead to one running out of sessions which will cause requests to fail. That is the reason I removed the async version. You almost always want to block for this method to finish.

…

On Mon, May 1, 2017 at 3:47 PM, Daniel Compton ***@***.***> wrote: Thanks for updating the javadocs. I'm still a little uneasy about having blocking RPC calls in the shutdown path though. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2026 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATdefzp5iSCWxKfhSyeI8DA2rGGLmXRTks5r1mDsgaJpZM4NNavX> .

coveralls · 2017-05-01T23:21:45Z

Coverage decreased (-0.009%) to 80.952% when pulling 1d867bf on vkedia:session-pool-exception into 9623994 on GoogleCloudPlatform:master.

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

+    private static final long serialVersionUID = 1451131180314064914L;
+
+    private LeakedSessionException() {
+      super("Session was checked out from the pool at:");


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


+  // Exception class used just to track the stack trace at the point when a session was handed out
+  // from the pool.
+  private final class LeakedSessionException extends Exception {


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

+
+    void close() {
+      scheduledFuture.cancel(false);
+      if (!running) {


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

+   * unused sessions piling up on the backend.
   */
-  ApiFuture<Void> closeAsync();
+  void close();


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

+      throw SpannerExceptionFactory.newSpannerException(e);
+    }
+    for (ManagedChannel channel : getOptions().getRpcChannels()) {
+      channel.shutdown();


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

-                  releaseSession(new PooledSession(session));
+                  Preconditions.checkState(totalSessions() <= options.getMaxSessions() - 1);
+                  PooledSession pooledSession = new PooledSession(session);
+                  allSessions.put(session.getName(), pooledSession);


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


 /** Options for the session pool used by {@code DatabaseClient}. */
 public class SessionPoolOptions {
+  private static final int DEFAULT_MAX_SESSIONS = 2000;


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

 /** Options for the session pool used by {@code DatabaseClient}. */
 public class SessionPoolOptions {
+  private static final int DEFAULT_MAX_SESSIONS = 2000;
+  private static final ActionOnExhaustion DEFAULT_ACTION = ActionOnExhaustion.FAIL;


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

+/**
+ * An interface for Cloud Spanner. Typically, there would only be one instance of this for the
+ * lifetime of the application which must be closed by invoking {@link #close()} when it is no
+ * longer needed.


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

   * Closes all the clients associated with this instance and frees up all the resources. This
-   * method does not block. Return future will complete when cleanup is done. TODO(user): Add
-   * logging and tracking of leaked sessions.
+   * method will block till it can clean up all the resources. Specifically, it deletes all the


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

+      writePreparedSessions.clear();
+      for (final PooledSession session : ImmutableList.copyOf(allSessions.values())) {
+        if (session.leakedException != null) {
+          logger.log(Level.WARNING, "Leaked session", session.leakedException);


vkedia · 2017-05-08T21:34:49Z

@tomayles @vam-google Please take a look again.

vam-google

LGTM

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

+   * unused sessions piling up on the backend.
   */
-  ApiFuture<Void> closeAsync();
+  void close();


coveralls · 2017-05-12T02:18:31Z

Changes Unknown when pulling 2b385eb on vkedia:session-pool-exception into ** on GoogleCloudPlatform:master**.

coveralls · 2017-05-12T03:05:08Z

Changes Unknown when pulling 2b385eb on vkedia:session-pool-exception into ** on GoogleCloudPlatform:master**.

coveralls · 2017-05-12T17:08:04Z

Changes Unknown when pulling 710701b on vkedia:session-pool-exception into ** on GoogleCloudPlatform:master**.

vkedia added 7 commits May 1, 2017 10:12

Formatting fixes

3a308a6

Merge branch 'master' into session-pool-exception

935407d

Added license headers

1bb879b

Changed Instant type

5fa0f0f

Track where the session was closed at

125753e

Track expired sessions

c9d56df

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 1, 2017

vkedia requested a review from vam-google May 1, 2017 20:45

vkedia closed this May 1, 2017

Fixed codacy commant and some other stylistic things

ae8860f

vkedia reopened this May 1, 2017

This was referenced May 1, 2017

Spanner service never completes a closeAsync when there is an unused ReadContext #1986

Closed

Spanner - Running Spanner on AppEngine SDK throws an NPE #1856

Closed

Shade Guava in google-cloud-spanner #1802

Closed

danielcompton reviewed May 1, 2017

View reviewed changes

Fixed javadocs

531f73c

Fixed javadocs

1d867bf

vam-google reviewed May 2, 2017

View reviewed changes

tomayles reviewed May 3, 2017

View reviewed changes

vkedia added 3 commits May 8, 2017 14:29

Fixed review comment

90d5ef2

Formatting fixes

ac88a50

Merge branch 'master' into session-pool-exception

2677fd0

vam-google approved these changes May 9, 2017

View reviewed changes

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

* unused sessions piling up on the backend.

*/

ApiFuture<Void> closeAsync();

void close();

This comment was marked as spam.

Sign in to view

tomayles approved these changes May 10, 2017

View reviewed changes

vkedia added 4 commits May 10, 2017 11:37

Merge branch 'master' into session-pool-exception

a5f74aa

Fixed a race between health checker and pool closure logic

18b591a

Merge branch 'master' into session-pool-exception

f8b8953

Formatting fixes

2b385eb

Changed log to be FINE as it was too spammy

710701b

vkedia merged commit 8db50cc into googleapis:master May 12, 2017

vkedia deleted the session-pool-exception branch May 12, 2017 17:43

suztomo pushed a commit to suztomo/google-cloud-java that referenced this pull request Mar 23, 2026

docs: add sample for MyBatis with GoogleSQL (googleapis#2026)

787096e

Conversation

vkedia commented May 1, 2017

Uh oh!

vkedia commented May 1, 2017

Uh oh!

coveralls commented May 1, 2017

Uh oh!

vkedia commented May 1, 2017

Uh oh!

coveralls commented May 1, 2017

Uh oh!

danielcompton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

coveralls commented May 1, 2017

Uh oh!

danielcompton commented May 1, 2017

Uh oh!

vkedia commented May 1, 2017 via email

Uh oh!

coveralls commented May 1, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

vkedia commented May 8, 2017

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

danielcompton left a comment •

edited

Loading