feat: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704

arpan14 · 2023-10-27T10:23:56Z

For fixing - #2659

Base Refactoring Done

Refactored lastUseTime parameter from PooledSession to SessionImpl class.
- A lot of existing tests had to be refactored to now work with the above refactoring.
FakeClock earlier was only being used within SessionPool class. Now it has to be used with multiple classes since lastUseTime was moved to SessionImpl class. Now moved it to a new class due to multiple usages.

New Changes Made

Modified TransactionRunnerImpl and AbstractReadContext class.
Introduced two new unit tests in DatabaseClientImplTest that now ensures that lastUseTime now gets updated with every RPC. Previously these tests were failing.

…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206

fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.

…ssionImpl class. Fix updation of the parameter for chained RPCs within one transaction.

…nPoolMaintainerTest.

…t. Add a couple of unit tests for testing the new behaviour.

…k instance.

…Fix the unit test to test this change.

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

olavloite · 2023-10-30T10:36:25Z

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

@@ -73,6 +73,8 @@ abstract static class Builder<B extends Builder<?, T>, T extends AbstractReadCon
    private QueryOptions defaultQueryOptions = SpannerOptions.Builder.DEFAULT_QUERY_OPTIONS;
    private ExecutorProvider executorProvider;

+    private Clock clock;


nit: maybe set this by default to = new Clock() and remove the null check in the AbstractReadContext constructor?

Sure, this is the clock field within the builder class. I have set it to = new Clock() by default at the member definition (L402)

I actually meant setting this one (so Builder.clock) by default to = new Clock(). That way, the clock in the AbstractReadContext can be final.

olavloite · 2023-10-30T10:39:49Z

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

@@ -416,6 +425,7 @@ void initTransaction() {
    this.defaultQueryOptions = builder.defaultQueryOptions;
    this.span = builder.span;
    this.executorProvider = builder.executorProvider;
+    this.clock = builder.clock == null ? new Clock() : builder.clock;


nit: see above, we could remove this null check by setting a default in the builder. Otherwise, prefer the use of com.google.common.base.MoreObjects.firstNonNull(..)

Using this.clock = firstNonNull(builder.clock, this.clock);

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

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

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

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

olavloite · 2023-10-30T10:58:24Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

+                  Options.priority(RpcPriority.HIGH))) {
+            while (resultSet.next()) {}
+          }
+          poolMaintainerClock.currentTimeMillis += Duration.ofMillis(2050).toMillis();


Just to be sure: Because the IdleTimeThreshold is 3 seconds, this means that no session will be cleaned up, right?

Without the code change, this unit test was failing. We are asserting the number of sessions removed at the end.

assertEquals(0, client.pool.numLeakedSessionsRemoved());

In this test, we have two read RPCs executed. After the first RPC we advance the clock by 1050ms and after the second RPC we advance the clock by 2050ms. This means the overall transaction took > 3s. Hence the current logic would recycle the session thinking its a long running transaction. This is exactly the bug we want to fix.

Post making the code changes, we are ensuring that the session's lastUseTime is getting correctly updated per RPC. And hence, we won't be recycling this session. This again gets tested through the assert statement.

Thanks for the detailed explanation, that sounds good to me.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

olavloite · 2023-10-30T11:01:23Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolStressTest.java

+          }
+
+          @Override
+          public void prepareReadWriteTransaction() {


Probably for a different PR, but this seems like a left-over from before InlineBeginTransaction.

…redundant updates from SessionPool class.

… more unit tests.

…text.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

olavloite

LGTM, with a minor nit on the Builder classes for AbstractReadContext and TransactionContext

olavloite · 2023-11-02T08:25:04Z

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

@@ -73,6 +73,8 @@ abstract static class Builder<B extends Builder<?, T>, T extends AbstractReadCon
    private QueryOptions defaultQueryOptions = SpannerOptions.Builder.DEFAULT_QUERY_OPTIONS;
    private ExecutorProvider executorProvider;

+    private Clock clock;


I actually meant setting this one (so Builder.clock) by default to = new Clock(). That way, the clock in the AbstractReadContext can be final.

olavloite · 2023-11-02T08:25:30Z

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

@@ -392,6 +399,8 @@ void initTransaction() {
  private final int defaultPrefetchChunks;
  private final QueryOptions defaultQueryOptions;

+  private Clock clock = new Clock();


See above, I think this can be made final

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

olavloite · 2023-11-02T09:32:00Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

+                  Options.priority(RpcPriority.HIGH))) {
+            while (resultSet.next()) {}
+          }
+          poolMaintainerClock.currentTimeMillis += Duration.ofMillis(2050).toMillis();


Thanks for the detailed explanation, that sounds good to me.

…ansactionRunnerImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

arpan14 and others added 13 commits February 8, 2023 12:22

fix: prevent illegal negative timeout values into thread sleep() meth…

edc5bbf

…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206

Merge pull request #1 from arpan14/retryerror

49a85df

fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.

Fixing lint issues.

4cd497b

Merge branch 'googleapis:main' into main

4a6aa8e

Merge branch 'googleapis:main' into main

b2aa09d

Merge branch 'googleapis:main' into main

8d6d71e

Merge branch 'googleapis:main' into main

77e6e7d

Merge branch 'googleapis:main' into main

e8b7fad

Merge branch 'googleapis:main' into main

8aa84e1

refactor: move session lastUseTime parameter from PooledSession to Se…

922f324

…ssionImpl class. Fix updation of the parameter for chained RPCs within one transaction.

chore: add clock instances in callees of SessionImpl.

b544080

chore: partially fix failing unit tests in SessionPoolTest and Sessio…

9862265

…nPoolMaintainerTest.

chore: fix failing tests in SessionPoolStressTest.

5e5f769

arpan14 requested a review from a team as a code owner October 27, 2023 10:23

product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Oct 27, 2023

arpan14 mentioned this pull request Oct 27, 2023

Session lastUseTime parameter not updated correctly for a Transaction #2659

Closed

arpan14 added 8 commits October 27, 2023 18:58

chore: update lastUseTime for methods in SessionPoolTransactionContex…

a385ceb

…t. Add a couple of unit tests for testing the new behaviour.

chore: lint errors.

fd3bb41

chore: fix tests in DatabaseClientImplTest by passing the mocked cloc…

4864053

…k instance.

fix: update session lastUseTime field for AbstractReadContext class. …

f5b82fa

…Fix the unit test to test this change.

fix: failing tests in TransactionRunnerImplTest.

73f0192

fix: failing test in SessionPoolMaintainerTest.

ff32178

refactor: move FakeClock to a new class.

1316579

refactor: move Clock to a new class.

80dd971

arpan14 requested a review from olavloite October 30, 2023 08:31

asthamohta reviewed Oct 30, 2023

View reviewed changes

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java Show resolved Hide resolved

olavloite reviewed Oct 30, 2023

View reviewed changes

arpan14 added 2 commits October 31, 2023 13:46

chore: resolving PR comments.

1acd645

chore: address review comments.

6af8187

arpan14 added 2 commits October 31, 2023 15:41

chore: updating lastUseTime state in TransactionRunnerImpl. Removing …

999a39b

…redundant updates from SessionPool class.

chore: remove redundant update statements from SessionPool class. Add…

ec80d6a

… more unit tests.

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Nov 1, 2023

arpan14 added 2 commits November 1, 2023 15:42

chore: add more tests for TransactionRunner.

6cdef81

chore: remove dead code from constructor of SessionPoolTransactionCon…

593a10b

…text.

arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 1, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 1, 2023

🦉 Updates from OwlBot post-processor

ced1e06

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

gcf-owl-bot bot requested a review from a team as a code owner November 1, 2023 13:04

asthamohta approved these changes Nov 2, 2023

View reviewed changes

olavloite approved these changes Nov 2, 2023

View reviewed changes

arpan14 and others added 4 commits November 2, 2023 17:58

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Tr…

0485aee

…ansactionRunnerImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Tr…

c4163d8

…ansactionRunnerImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Tr…

b75b19f

…ansactionRunnerImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

chore: fixing precondition errors due to null clock.

86327e2

arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 3, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 3, 2023

arpan14 merged commit e75a281 into googleapis:main Nov 3, 2023
22 checks passed

arpan14 deleted the session-lastusetime-refactor branch November 3, 2023 06:40

release-please bot mentioned this pull request Nov 3, 2023

chore(main): release 6.53.0 #2706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704

feat: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704

arpan14 commented Oct 27, 2023 •

edited

olavloite Oct 30, 2023

arpan14 Oct 31, 2023

olavloite Nov 2, 2023

olavloite Oct 30, 2023

arpan14 Oct 31, 2023

olavloite Oct 30, 2023

arpan14 Oct 31, 2023

olavloite Nov 2, 2023

olavloite Oct 30, 2023

olavloite left a comment

olavloite Nov 2, 2023

olavloite Nov 2, 2023

olavloite Nov 2, 2023

feat: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704

feat: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704

Conversation

arpan14 commented Oct 27, 2023 • edited

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 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

arpan14 commented Oct 27, 2023 •

edited