ptrdom commented on code in PR #606:
URL:
https://github.com/apache/pekko-persistence-jdbc/pull/606#discussion_r4431119060
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/testkit/internal/SchemaUtilsImpl.scala:
##########
@@ -211,4 +214,19 @@ private[jdbc] object SchemaUtilsImpl {
SlickExtension(actorSystem).database(journalConfig)
}
+ /**
+ * Arranges for the database of `slickDb` to be closed when the actor system
terminates, when the caller owns that
+ * database. A database that is created for this call only (the default when
`use-shared-db` is not configured)
+ * would otherwise leak its connection pool, since nothing else holds on to
it. The database is kept open until the
+ * actor system terminates because closing it right after the script has
been applied would, for an H2 in-memory
+ * database, drop the schema that was just created.
+ */
+ private def closeOnTermination(slickDb: SlickDatabase)(
+ implicit actorSystem: ClassicActorSystemProvider): Database = {
+ if (slickDb.allowShutdown) {
+ actorSystem.classicSystem.registerOnTermination(slickDb.database.close())
Review Comment:
Small edge case: `registerOnTermination` throws
`RejectedExecutionException("ActorSystem already terminated.")` once the system
has fully terminated. Because `closeOnTermination` runs before the `Future` is
created, `createIfNotExists`/`dropIfExists`/`applyScript` on a terminated
system now throw synchronously instead of returning a failed `Future`. Moving
the call inside the `Future` would keep the failure in the `Future`:
```scala
Future(applyScriptWithSlick(fromClasspathAsString(fileToLoad), separator,
logger, closeOnTermination(slickDb)))(blockingEC)
```
Not a big deal either way, since the system is unusable at that point.
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/util/ConnectionPools.scala:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.util
+
+import org.apache.pekko
+import pekko.annotation.InternalApi
+import slick.jdbc.JdbcBackend.Database
+import slick.jdbc.hikaricp.HikariCPJdbcDataSource
+
+/**
+ * INTERNAL API
+ */
+@InternalApi
Review Comment:
Nit: these helpers live in `src/test` and are never published, so
`@InternalApi` and the `INTERNAL API` scaladoc don't add anything.
`private[jdbc]` on its own is enough. The same applies to
`RecordingSlickDatabaseProvider`.
##########
migrator/src/main/scala/org/apache/pekko/persistence/jdbc/migrator/JournalMigrator.scala:
##########
@@ -156,6 +157,13 @@ final case class JournalMigrator(profile:
JdbcProfile)(implicit system: ActorSys
_ <- newJournalQueries.TagTable ++= tagInserts
} yield ()
}
+
+ /**
+ * Closes the database that was opened for this migrator. It is a no-op when
the database is shared (configured with
+ * `use-shared-db`), since that one is owned by the actor system.
+ */
+ override def close(): Unit =
Review Comment:
Could the class-level scaladoc (and the one on `SnapshotMigrator`) say that
callers should `close()` the migrator once `migrate()`/`migrateAll()` has
finished? Otherwise nothing tells users the migrator now owns a resource.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
