Spanner: Re-create sessions that have been invalidated by the server #4734

olavloite · 2019-03-24T17:28:21Z

Sessions in the session pool that have been invalidated by the server may cause SessionNotFoundExceptions to bubble up to the user, even though the user has no control over the session management. When this occurs, the session should be removed from the SessionPool and a new session should be fetched from the SessionPool without the user having to intervene.
In case of a SessionNotFoundException being thrown while using a TransactionManager, the client library will replace the session with a new one from the pool, and trigger a restart of the transaction by throwing an AbortedException (it is not possible to replace the session without also restarting the transaction).

This change has been initiated on request from the Spanner team.

codecov · 2019-03-24T18:27:21Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #4734   +/-   ##
=========================================
  Coverage     50.36%   50.36%           
  Complexity    23665    23665           
=========================================
  Files          2233     2233           
  Lines        225856   225856           
  Branches      24956    24956           
=========================================
  Hits         113742   113742           
  Misses       103517   103517           
  Partials       8597     8597

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 aace1e5...a24bfb7. Read the comment docs.

sduskis

I have stylistic preferences. I'm not going to block this PR for those comments.

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

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

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

...ud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java

snehashah16

One high level comment, customers should ideally never see a SessionNotFound with this change. But in ur change, u only have 1 retry for error case. I would change it such that it is always retried for SessionNotFound

snehashah16 · 2019-04-04T17:16:32Z

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

+
+    private void maybeRecreateUnderlyingSession(SessionNotFoundException e) {
+      if (beforeFirstServerCall && allowDelegateRecreation) {
+        delegate = spanner.createSession(db);


could we get another session from the pool instead of creating a new one ?

I am afraid that the latency on this request will be too high when we internally retry.

snehashah16 · 2019-04-04T17:18:41Z

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

      try {
+        T res = method.get();


should this be after markUsed() ?

snehashah16 · 2019-04-04T17:22:44Z

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

+      } catch (SessionNotFoundException e) {
+        maybeRecreateUnderlyingSession(e);
+        lastException = null;
+        return internalRunWithSessionRetry(method);


there is a chance we got sessionNotFound here as well..

have u considered putting this in a loop ?
Pros: customers never see a SessionNotFound
Cons: high latency, but our timeouts should still take effect.

snehashah16 · 2019-04-04T17:25:35Z

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

+            } catch (SessionNotFoundException e) {
+              maybeRecreateUnderlyingSession(e);
+              TransactionRunner newRunner = delegate.readWriteTransaction();
+              result = newRunner.run(callable);


same as above, this could potentially still see a sessionNotFound (SNF)

sduskis · 2019-04-11T15:32:54Z

@olavloite, it looks like there are conflicts.

olavloite · 2019-04-11T18:21:12Z

@sduskis Yeah, I know. I'll start to rebase it once #4895 has been merged, as that will otherwise also cause further merge conflicts with this change.

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

kolea2 · 2019-04-22T17:23:22Z

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

+      try {
+        return callable.apply(session);
+      } catch (SessionNotFoundException e) {
+        session = pool.replaceReadWriteSession(e, session);


Following up from my last comment, if the session is READ, is this correct, as replaceReadWriteSession eventually returns 'getReadWriteSession()'?

Good point, it wasn't right, even though the method was never called with READ. But that was not correct either, as executePartitionedUpdate does not need a read/write session, as it always starts a new transaction (as it needs a special type of transaction).

kolea2 · 2019-04-22T19:39:33Z

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

@@ -188,7 +188,7 @@ public long executePartitionedUpdate(final Statement stmt) {
    Span span = tracer.spanBuilder(PARTITION_DML_TRANSACTION).startSpan();
    try (Scope s = tracer.withSpan(span)) {
      return runWithSessionRetry(
-          SessionMode.READ_WRITE,
+          SessionMode.READ, // PartitionedUpdate does not need a prepared tx.


Can we do this in a separate PR? Because this PR is quite large, I'd like to separate out the recreating sessions implementation from the refactorings/fixes.

I'll move it to a separate PR later this afternoon.

snehashah16 · 2019-05-02T22:01:21Z

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

+  }
+
+  @VisibleForTesting
+  int getNumberOfAvailableWritePreparedSessions() {


this is not DBClient's responsibility. I wouldn't expose it here

Method is now removed and the visibility of the session pool changed to package-private to make it visible for test cases.

snehashah16 · 2019-05-02T22:03:57Z

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

@@ -69,7 +99,7 @@ public Timestamp writeAtLeastOnce(Iterable<Mutation> mutations) throws SpannerEx
  public ReadContext singleUse() {
    Span span = tracer.spanBuilder(READ_ONLY_TRANSACTION).startSpan();
    try (Scope s = tracer.withSpan(span)) {
-      return pool.getReadSession().singleUse();
+      return getReadSession().singleUse();


curious whats the behavior with read sessions seeing SNF errors ?

The SNF is detected at the first actual database request. For read-only transactions (both single use and actual read transactions), that is at the first query. The query is sent to the server at the first call to ResultSet#next(), so the SNF for a read transaction is handled here:

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

Line 129 in 1b61e28

return internalNext();

snehashah16 · 2019-05-02T22:06:09Z

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

    } catch (RuntimeException e) {
      TraceUtil.endSpanWithFailure(span, e);
      throw e;
    }
  }

+  private <T> T runWithSessionRetry(Function<Session, T> callable) {
+    PooledSession session = getReadWriteSession();
+    while (true) {


Will this respect client side timeout values set ?
If our backend is down, and does not return any sessions -> whats the outcome of the customer application ?

The short answer: It will behave the same as now, and the behavior will depend on whether the pool has a session ready to be returned or not. The runWithSessionRetry will only retry on the specific condition of a SessionNotFoundException being returned.

There are two possible scenarios:

The application requests a session and the session pool has a session that it thinks is valid. This session is handed to the application and the application tries to execute the transaction. This will cause the query or begin transaction statement to time out.

The pool does not have a session available that can be returned and starts the creation of a new session asynchronously. The requesting thread is placed in a waiter. The create session request will fail and notify the waiter of the error. The waiting thread will return with the create session error.

Both of the above will not be considered a SessionNotFoundException by the runWithSessionRetry method, and the method will return with an error.

Replaced while(true) loops with internal methods. Added method for creating ReadOnlyTransactions.

When a SessionNotFoundException is thrown, a new session is now picked from the pool, instead of creating a new session on the server. This reduces latency in calls that get an invalidated session from the pool. It does however also require that the retry-logic for SessionNotFound is put in a loop, as the second session returned from the pool could also possibly be an invalidated session.

When a SessionNotFoundException occurs, a new session should be requested from the SessionPool instead of creating a new one.

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

olavloite marked this pull request as ready for review March 26, 2019 10:39

olavloite requested a review from a team as a code owner March 26, 2019 10:39

olavloite changed the title ~~[WIP] Spanner: Re-create sessions that have been invalidated by the server~~ Spanner: Re-create sessions that have been invalidated by the server Mar 26, 2019

sduskis reviewed Mar 26, 2019

View reviewed changes

sduskis assigned kolea2 Mar 26, 2019

sduskis requested a review from kolea2 March 26, 2019 16:33

sduskis added the api: spanner Issues related to the Spanner API. label Mar 26, 2019

kolea2 reviewed Mar 27, 2019

View reviewed changes

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

kolea2 reviewed Mar 27, 2019

View reviewed changes

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java Outdated Show resolved Hide resolved

kolea2 reviewed Mar 27, 2019

View reviewed changes

...e-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java Outdated Show resolved Hide resolved

kolea2 reviewed Mar 27, 2019

View reviewed changes

...ud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java Outdated Show resolved Hide resolved

yoshi-automation added the 🚨 This issue needs some love. label Mar 31, 2019

snehashah16 reviewed Apr 4, 2019

View reviewed changes

olavloite force-pushed the spanner-retry-on-invalidated-session branch from 10ccbb2 to a05d6f1 Compare April 9, 2019 10:35

sduskis added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Apr 11, 2019

olavloite changed the title ~~Spanner: Re-create sessions that have been invalidated by the server~~ [WIP] Spanner: Re-create sessions that have been invalidated by the server Apr 11, 2019

olavloite force-pushed the spanner-retry-on-invalidated-session branch 2 times, most recently from e0224e2 to a63929e Compare April 16, 2019 05:27

olavloite changed the title ~~[WIP] Spanner: Re-create sessions that have been invalidated by the server~~ Spanner: Re-create sessions that have been invalidated by the server Apr 16, 2019

olavloite requested review from kolea2, snehashah16 and sduskis April 16, 2019 08:51

kolea2 reviewed Apr 22, 2019

View reviewed changes

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

kolea2 reviewed Apr 22, 2019

View reviewed changes

snehashah16 reviewed May 2, 2019

View reviewed changes

kolea2 approved these changes May 9, 2019

View reviewed changes

olavloite added 22 commits May 9, 2019 17:24

refactor SpannerImpl: move SessionImpl to separate file

41dc280

fixed merge conflicts

c443e82

changed references after rebase on master

cfe198d

re-create sessions that have been invalidated by the server

3f9a82b

increase timeout for integration tests

d4863e4

increase of integration test timeout

b326110

added test cases for sessionnotfound

4dab5c5

processed review comments

97cb3cc

Replaced while(true) loops with internal methods. Added method for creating ReadOnlyTransactions.

added missing retry logic for direct operations on the session

466dd56

changed to use instance variable for better readability

c9e2030

removed change of test env config

64bbd91

try to fix flaky test case

f151231

request new session from the pool instead of creating a new session

4976f55

When a SessionNotFoundException occurs, a new session should be requested from the SessionPool instead of creating a new one.

rebased and fixed merge conflicts

f74cdb9

added tests for methods on DatabaseClient

bcd474b

added project to builder to fix test case

8b98662

cleanup and add documentation

f8d8075

fixed formatting

6df4f2b

replace a read session with a new read session and not read/write

d4717a0

moved fix for executePartitionedUpdate to separate PR

054bbfc

make session pool accessible for test cases

55ec5ec

olavloite force-pushed the spanner-retry-on-invalidated-session branch from 829b657 to 55ec5ec Compare May 9, 2019 16:17

olavloite merged commit 7d47fce into googleapis:master May 13, 2019

chingor13 mentioned this pull request May 15, 2019

Release google-cloud-java v0.92.0 #5189

Merged

taka-oyama mentioned this pull request May 29, 2023

[Spanner] Re-create sessions that have been invalidated by the server googleapis/google-cloud-php#6284

Closed

taka-oyama mentioned this pull request Aug 23, 2023

[6.0] feat: remove Session Not Found Error Mode colopl/laravel-spanner#132

Merged

1 task

MostafaOmar98 mentioned this pull request Jan 6, 2024

Server-side deleted sessions (specifically for being older than 28 days) are not refreshed in PingingPool googleapis/python-spanner#1068

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spanner: Re-create sessions that have been invalidated by the server #4734

Spanner: Re-create sessions that have been invalidated by the server #4734

olavloite commented Mar 24, 2019 •

edited

Loading

codecov bot commented Mar 24, 2019 •

edited

Loading

sduskis left a comment

snehashah16 left a comment

snehashah16 Apr 4, 2019

snehashah16 Apr 4, 2019

snehashah16 Apr 4, 2019

snehashah16 Apr 4, 2019

sduskis commented Apr 11, 2019

olavloite commented Apr 11, 2019

kolea2 Apr 22, 2019

olavloite Apr 22, 2019

kolea2 Apr 22, 2019

olavloite Apr 23, 2019

snehashah16 May 2, 2019

olavloite May 5, 2019

snehashah16 May 2, 2019

olavloite May 5, 2019

snehashah16 May 2, 2019

olavloite May 5, 2019

Spanner: Re-create sessions that have been invalidated by the server #4734

Spanner: Re-create sessions that have been invalidated by the server #4734

Conversation

olavloite commented Mar 24, 2019 • edited Loading

codecov bot commented Mar 24, 2019 • edited Loading

Codecov Report

sduskis left a comment

Choose a reason for hiding this comment

snehashah16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sduskis commented Apr 11, 2019

olavloite commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Mar 24, 2019 •

edited

Loading

codecov bot commented Mar 24, 2019 •

edited

Loading