Skip to content

fix: do not delete session in close method for BatchReadOnlyTransactionImpl #1688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 15, 2022
Merged

fix: do not delete session in close method for BatchReadOnlyTransactionImpl #1688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f1ad68d
fix: do not delete session in close method for BatchReadOnlyTransacti…
rajatbhatta Feb 14, 2022
cff4c73
🦉 Updates from OwlBot
gcf-owl-bot[bot] Feb 14, 2022
de25691
Incorporate review comments
rajatbhatta Feb 15, 2022
6f8a1e3
Incorporate review comments.
rajatbhatta Feb 15, 2022
f591fa2
🦉 Updates from OwlBot
gcf-owl-bot[bot] Feb 15, 2022
719592f
Merge branch 'java-spanner-issue-1651-fix' of https://github.com/goog…
gcf-owl-bot[bot] Feb 15, 2022
64cb4b7
🦉 Updates from OwlBot
gcf-owl-bot[bot] Feb 15, 2022
c5137fc
Merge branch 'java-spanner-issue-1651-fix' of https://github.com/goog…
gcf-owl-bot[bot] Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions google-cloud-spanner/clirr-ignored-differences.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
<className>com/google/cloud/spanner/connection/Connection</className>
<method>com.google.cloud.spanner.Dialect getDialect()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/BatchReadOnlyTransaction</className>
<method>void cleanup()</method>
</difference>
<difference>
<differenceType>8001</differenceType>
<className>com/google/cloud/spanner/connection/StatementParser</className>
Expand Down
9 changes: 7 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,14 @@ public ResultSet execute(Partition partition) throws SpannerException {
partition.getPartitionToken());
}

/**
* Closes the session as part of the cleanup. It is the responsibility of the caller to make a
* call to this method once the transaction completes execution across all the channels (which
* is understandably hard to identify). It is okay if the caller does not call the method
* because the backend will anyways clean up the unused session.
*/
@Override
public void close() {
super.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, won't this break all the existing clients that are relying on the close method to return the session to the pool? (won't this possibly cause a session pool exhaustion for existing clients?)

cc @olavloite

Copy link
Contributor Author

@rajatbhatta rajatbhatta Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As BatchReadOnlyTransactionImpl is an AutoCloseable (via MultiUseReadOnlyTransaction->AbstractReadContext->ReadContext->AutoClosable), having it within a try-with-resources construct allows automatic closing of the transaction resource, which leads the underlying session to be released to the session pool. IMO, it should not lead to session pool exhaustion.

Having an overridden close method with just super.close() won't make sense, as the super's close method will anyways be called during automatic closing.

@olavloite : Please do share your viewpoint as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thiagotnunes No, the BatchClientImpl does not use sessions from the pool. It gets its session in one of two possible ways:

  1. It is the client that creates the BatchReadOnlyTransaction: It creates a session here.
  2. It is a client that receives a batch transaction id from a different client: It just references the already existing session that was created by the other client here.

So it means that we will create one session that will not (always) be deleted. We choose to do it this way because we do not know how long all clients will need to finish reading, and the session must be kept alive as long as there are clients still reading. The session will eventually be garbage collected by the backend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh got it, thanks for the explanation!

public void cleanup() {
session.close();
}
}
Expand Down
8 changes: 8 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,12 @@ List<Partition> partitionQuery(
* BatchTransactionId guarantees the subsequent read/query to be executed at the same timestamp.
*/
BatchTransactionId getBatchTransactionId();

/**
* Closes the session as part of the cleanup. It is the responsibility of the caller to make a
* call to this method once the transaction completes execution across all the channels (which is
* understandably hard to identify). It is okay if the caller does not call the method because the
* backend will anyways clean up the unused session.
*/
default void cleanup() {}
}
21 changes: 13 additions & 8 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import com.google.common.util.concurrent.SettableFuture;
import com.google.protobuf.AbstractMessage;
import com.google.spanner.v1.CommitRequest;
import com.google.spanner.v1.DeleteSessionRequest;
import com.google.spanner.v1.ExecuteBatchDmlRequest;
import com.google.spanner.v1.ExecuteSqlRequest;
import com.google.spanner.v1.ExecuteSqlRequest.QueryMode;
Expand Down Expand Up @@ -1630,16 +1631,20 @@ public void testBackendPartitionQueryOptions() {
try (ResultSet rs = transaction.execute(partitions.get(0))) {
// Just iterate over the results to execute the query.
while (rs.next()) {}
} finally {
transaction.cleanup();
}
// Check that the last query was executed using a custom optimizer version and statistics
// package.
// Check if the last query executed is a DeleteSessionRequest and the second last query
// executed is a ExecuteSqlRequest and was executed using a custom optimizer version and
// statistics package.
List<AbstractMessage> requests = mockSpanner.getRequests();
assertThat(requests).isNotEmpty();
assertThat(requests.get(requests.size() - 1)).isInstanceOf(ExecuteSqlRequest.class);
ExecuteSqlRequest request = (ExecuteSqlRequest) requests.get(requests.size() - 1);
assertThat(request.getQueryOptions()).isNotNull();
assertThat(request.getQueryOptions().getOptimizerVersion()).isEqualTo("1");
assertThat(request.getQueryOptions().getOptimizerStatisticsPackage())
assert requests.size() >= 2 : "required to have at least 2 requests";
assertThat(requests.get(requests.size() - 1)).isInstanceOf(DeleteSessionRequest.class);
assertThat(requests.get(requests.size() - 2)).isInstanceOf(ExecuteSqlRequest.class);
ExecuteSqlRequest executeSqlRequest = (ExecuteSqlRequest) requests.get(requests.size() - 2);
assertThat(executeSqlRequest.getQueryOptions()).isNotNull();
assertThat(executeSqlRequest.getQueryOptions().getOptimizerVersion()).isEqualTo("1");
assertThat(executeSqlRequest.getQueryOptions().getOptimizerStatisticsPackage())
.isEqualTo("custom-package");
}
}
Expand Down