fix: do not delete session in close method for BatchReadOnlyTransacti…

…onImpl (#1688)

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

* 🦉 Updates from OwlBot

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

* Incorporate review comments

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

* Incorporate review comments.

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

* 🦉 Updates from OwlBot

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

* 🦉 Updates from OwlBot

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

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>

google-cloud-spanner/clirr-ignored-differences.xml

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

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 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();
+    public void cleanup() {
       session.close();
     }
   }

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

@@ -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() {}
 }

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<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");
     }
   }