From f1ad68d687794cca56376ebfc4a0e915434c4af5 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta Date: Mon, 14 Feb 2022 13:17:54 +0530 Subject: [PATCH 1/6] fix: do not delete session in close method for BatchReadOnlyTransactionImpl - remove session.close() from close() method implementation in BatchReadOnlyTransactionImpl class. - add a cleanup() method to BatchReadOnlyTransaction interface and give user the control to delete session when the session is no longer in use. - add tests for txn.cleanup() method. --- .../clirr-ignored-differences.xml | 5 +++++ .../google/cloud/spanner/BatchClientImpl.java | 9 ++++++-- .../spanner/BatchReadOnlyTransaction.java | 8 +++++++ .../cloud/spanner/DatabaseClientImplTest.java | 21 ++++++++++++------- 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/google-cloud-spanner/clirr-ignored-differences.xml b/google-cloud-spanner/clirr-ignored-differences.xml index c750b2a7f40..f65bfd94332 100644 --- a/google-cloud-spanner/clirr-ignored-differences.xml +++ b/google-cloud-spanner/clirr-ignored-differences.xml @@ -6,6 +6,11 @@ com/google/cloud/spanner/connection/Connection com.google.cloud.spanner.Dialect getDialect() + + 7012 + com/google/cloud/spanner/BatchReadOnlyTransaction + void cleanup() + 8001 com/google/cloud/spanner/connection/StatementParser diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java index c84bef77cf8..82d7f427ac1 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java @@ -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 + * 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(); + public void cleanup() { session.close(); } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java index 9a9613f3247..6b47126f7ad 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java @@ -197,4 +197,12 @@ List 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 + * 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() {} } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java index 20e8c6b8ea6..773124ca6f1 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java @@ -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; @@ -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 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"); } } From cff4c73dfbb45494c76111fc7b603ca7cef2dc86 Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Mon, 14 Feb 2022 08:02:12 +0000 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .../google/cloud/spanner/BatchReadOnlyTransaction.java | 8 ++++---- .../com/google/cloud/spanner/DatabaseClientImplTest.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java index 6b47126f7ad..86fc1941673 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java @@ -199,10 +199,10 @@ List partitionQuery( BatchTransactionId getBatchTransactionId(); /** - * Closes the session as part of the cleanup. It is the responsibility of the caller to make - * 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. + * Closes the session as part of the cleanup. It is the responsibility of the caller to make 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() {} } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java index 773124ca6f1..21c9d3eaa77 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java @@ -1631,7 +1631,7 @@ public void testBackendPartitionQueryOptions() { try (ResultSet rs = transaction.execute(partitions.get(0))) { // Just iterate over the results to execute the query. while (rs.next()) {} - } finally { + } finally { transaction.cleanup(); } // Check if the last query executed is a DeleteSessionRequest and the second last query From de25691d84a20ce2541b5f836c516a9d9d7fb3f5 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta <93644539+rajatbhatta@users.noreply.github.com> Date: Tue, 15 Feb 2022 07:06:18 +0000 Subject: [PATCH 3/6] Incorporate review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Knut Olav Løite --- .../java/com/google/cloud/spanner/BatchReadOnlyTransaction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java index 86fc1941673..38160c546d8 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java @@ -199,7 +199,7 @@ List partitionQuery( BatchTransactionId getBatchTransactionId(); /** - * Closes the session as part of the cleanup. It is the responsibility of the caller to make call + * 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. From 6f8a1e35638e723c0f1497658a1b09199391b127 Mon Sep 17 00:00:00 2001 From: Rajat Bhatta <93644539+rajatbhatta@users.noreply.github.com> Date: Tue, 15 Feb 2022 07:06:37 +0000 Subject: [PATCH 4/6] Incorporate review comments. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Knut Olav Løite --- .../src/main/java/com/google/cloud/spanner/BatchClientImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java index 82d7f427ac1..fe31a3b0101 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java @@ -215,7 +215,7 @@ public ResultSet execute(Partition partition) throws SpannerException { } /** - * Closes the session as part of the cleanup. It is the responsibility of the caller to make + * 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. From f591fa2cc037fc46dde287ad374978e3b4bbf9ec Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Tue, 15 Feb 2022 07:08:46 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .../com/google/cloud/spanner/BatchReadOnlyTransaction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java index 38160c546d8..03b08a11730 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java @@ -199,8 +199,8 @@ List partitionQuery( 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 + * 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. */ From 64cb4b7459b80d7b7d9a1a1786ff14770aa2ea9a Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Tue, 15 Feb 2022 07:08:59 +0000 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .../com/google/cloud/spanner/BatchReadOnlyTransaction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java index 38160c546d8..03b08a11730 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchReadOnlyTransaction.java @@ -199,8 +199,8 @@ List partitionQuery( 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 + * 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. */